[AOT] Remove usage of Linq.Expressions and especially lambda compilation #4695

vitek-karas · 2023-07-26T12:36:11Z

Towards #3429

In AOT Expression.Lambda.Compile method is implemented via an interpreter. So it does work, but it's relatively slow. Additionally it's not fully NativeAOT compatible yet.

There are two places where it's used in the source code:

ExceptionProcessor uses it to access Marshal.GetExceptionPointers method which is not part of netstandard API. So in this case simply ifdef and on net6.0+ access the method directly (as it's visible in the net6.0 APIs) and fallback to the Expression.Lambda solution on down-level platforms. Since AOT/trimming is net6.0+ this fixes the problem.
ActivityInstrumentationHelper uses it to access two setters on the Activity class which are internal. Using reflection and then CreateDelegate will provide basically the same performance (a delegate which directly calls the setter) without a need for Expression.Lambda - so the code is simpler. Since the reflection is statically analyzable (all types are known, the property names are known) this code is fully trim and AOT compatible.

There's no impact on warning count because the .NET 7 ASP.NET has Linq.Expression dependency as well. On .NET 8 though this removes all warnings from Linq.Expressions.

@Yun-Ting @eerhardt

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

In AOT `Expression.Lambda.Compile` method is implemented via an interpreter. So it does work, but it's relatively slow. Additionally it's not fully NativeAOT compatible yet. There are two places where it's used in the source code: * `ExceptionProcessor` uses it to access `Marshal.GetExceptionPointers` method which is not part of netstandard API. So in this case simply ifdef and on net6.0+ access the method directly (as it's visible in the net6.0 APIs) and fallback to the Expression.Lambda solution on down-level platforms. Since AOT/trimming is net6.0+ this fixes the problem. * `ActivityInstrumentationHelper` uses it to access two setters on the `Activity` class which are internal. Using reflection and then `CreateDelegate` will provide basically the same performance (a delegate which directly calls the setter) without a need for `Expression.Lambda` - so the code is simpler. Since the reflection is statically analyzable (all types are known, the property names are known) this code is fully trim and AOT compatible. There's no impact on warning count because the .NET 7 ASP.NET has Linq.Expression dependency as well. On .NET 8 though this removes all warnings from Linq.Expressions.

eerhardt · 2023-07-26T13:00:05Z

src/Shared/ActivityInstrumentationHelper.cs

-        PropertyInfo sourcePropertyInfo = typeof(Activity).GetProperty("Source");
-        var body = Expression.Assign(Expression.Property(instance, sourcePropertyInfo), propertyValue);
-        return Expression.Lambda<Action<Activity, ActivitySource>>(body, instance, propertyValue).Compile();
+        return (Action<Activity, ActivitySource>)typeof(Activity).GetProperty("Source")


When I spoke to @tarekgh about this, and asked why these setters aren't exposed publicly (since OpenTelemetry needs to call them), he told me that these setters only need to be called on < net7.0 runtimes. If you look at the callsites, you'll see this is true for 2 of the 3 callsites:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Lines 195 to 198 in 0343116

#if !NET7_0_OR_GREATER

ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);

ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);

#endif

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

Lines 161 to 165 in 0343116

if (!IsNet7OrGreater)

{

ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);

ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);

}

And in Grpc, it always calls it:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs

Lines 124 to 127 in 0343116

if (activity.IsAllDataRequested)

{

ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);

ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);

I think the correct thing to do here is to not compile this code on net7.0+ TFMs, and then fix the 2 places above to have #if !NET7_0_OR_GREATER checks around them that don't already.

@utpilla do you agree to change GRPC to not call these on net7.0+?

There's an additional complication - it would require adding net7.0 as a TFM to Http and Grcp instrumentation assemblies. Http currently uses a boolean which is determined by looking at version of the System.Net.Http assembly, the trimmer would not be able to figure this out. I don't think we have a runtime property which the trimmer will know a constant value of and can be used to determine the version.

Additionally, Grpc package is currently netstandard2.0/netstandard2.1 only... we will probably have to change that anyway (to get the annotation attributes), but by default it would only get net6.0, not net7.0.

+@noahfalk as an "fyi".

CC @cijothomas @reyang

Older versions of aspnet and http client used to create the Activity objects the legacy way using new Activity. OpenTelemetry wanted to support these old versions of such platforms which needed to set Kind properly on such activity. This was the need for reflection. Aspnet and http client then moved (in .NET 7.0) to create the Activity object using ActivitySource which set the Kind correctly on the created Activity object so there was no need to do anything more with such objects or use the reflection.

I don't know gRPC scenarios. If it is same as aspnet then there is no need to call reflection on .NET 7.0+.

@cijothomas you were the one implementing that as I recall if you have more input here.

I don't know gRPC scenarios.

It looks like it isn't following this guidance of using ActivitySource.CreateActivity/StartActivity. 😢 For either it's own Activity:

https://github.com/grpc/grpc-dotnet/blob/0ab3ada34aa76740b436b3b1d320f2f406f82f50/src/Grpc.Net.Client/Internal/GrpcCall.cs#L823

nor its copy of the HTTP System.Net.Http.HttpRequestOut Activity:

https://github.com/grpc/grpc-dotnet/blob/0ab3ada34aa76740b436b3b1d320f2f406f82f50/src/Shared/TelemetryHeaderHandler.cs#L70

Maybe someone should log an issue on the gRPC client to move away from the "legacy way new Activity(...)" and use an ActivitySource. Then OpenTelemetry wouldn't need to use reflection here at all.

cc @JamesNK

grpc-dotnet was written for .NET Core 3, and ActivitySource didn't exist then. grpc-dotnet still targets .NET Core 3 + .NET 5, but I'm going to drop the unsupported targets for .NET 8.

Grpc.Net.Client also targets netstandard2.0, but ActivitySource is in the System.Diagnostics.DiagnosticSource NuGet package.

Issue: grpc/grpc-dotnet#2180 (comment)

PR: grpc/grpc-dotnet#2244

src/OpenTelemetry/Trace/ExceptionProcessor.cs

utpilla

Looks good.

Things to do outside of this PR:

Update targets for GrpcNetClient instrumentation library
Check if GrpcNetClient instrumentation library can be updated to listen to ActivitySource on net7+ targets

codecov · 2023-07-28T00:31:46Z

Codecov Report

Merging #4695 (4985d8d) into main (8c7173f) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4695      +/-   ##
==========================================
- Coverage   85.14%   85.10%   -0.04%     
==========================================
  Files         314      314              
  Lines       12735    12722      -13     
==========================================
- Hits        10843    10827      -16     
- Misses       1892     1895       +3

Files Changed	Coverage Δ
src/OpenTelemetry/Trace/ExceptionProcessor.cs	`100.00% <100.00%> (+8.33%)`	⬆️
src/Shared/ActivityInstrumentationHelper.cs	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

src/OpenTelemetry/Trace/ExceptionProcessor.cs

vitek-karas · 2023-07-28T14:32:04Z

I don't understand the test failure - it passes locally just fine.

eerhardt · 2023-07-28T15:26:47Z

Why doesn't this change decrease the AOT warning count?

opentelemetry-dotnet/test/OpenTelemetry.AotCompatibility.Tests/AotCompatibilityTests.cs

Line 88 in 8c7173f

Assert.Equal(30, warnings.Count());

Are there still usages of System.Linq.Expressions hanging around?

vitek-karas · 2023-07-28T15:28:36Z

Why doesn't this change decrease the AOT warning count?

It does on .NET 8. But the test runs on .NET 7 and in .NET 7 ASP.NET uses Linq.Expression is certain places which pull it in -> causing the same warnings.

I verified that there are no Linq.Expression usages from OTel after this change.

eerhardt

Thanks for fixing this, @vitek-karas. LGTM

vitek-karas marked this pull request as ready for review July 26, 2023 12:36

vitek-karas requested a review from a team July 26, 2023 12:36

eerhardt reviewed Jul 26, 2023

View reviewed changes

utpilla reviewed Jul 27, 2023

View reviewed changes

src/OpenTelemetry/Trace/ExceptionProcessor.cs Outdated Show resolved Hide resolved

utpilla reviewed Jul 27, 2023

View reviewed changes

src/OpenTelemetry/Trace/ExceptionProcessor.cs Outdated Show resolved Hide resolved

utpilla added 2 commits July 27, 2023 17:15

Update src/OpenTelemetry/Trace/ExceptionProcessor.cs

1fdd8ee

Update src/OpenTelemetry/Trace/ExceptionProcessor.cs

d041885

utpilla approved these changes Jul 28, 2023

View reviewed changes

Merge branch 'main' into RemoveLinqExp

132003d

JamesNK mentioned this pull request Jul 28, 2023

Remove .NET Core 3 and .NET 5 targets grpc/grpc-dotnet#2180

Closed

utpilla reviewed Jul 28, 2023

View reviewed changes

src/OpenTelemetry/Trace/ExceptionProcessor.cs Outdated Show resolved Hide resolved

Use the API directly even on netfx

0a2a105

Merge branch 'main' into RemoveLinqExp

4985d8d

eerhardt approved these changes Jul 28, 2023

View reviewed changes

Yun-Ting approved these changes Jul 28, 2023

View reviewed changes

utpilla approved these changes Jul 28, 2023

View reviewed changes

utpilla merged commit 41daba4 into open-telemetry:main Jul 28, 2023

JamesNK mentioned this pull request Aug 15, 2023

Update Grpc.Net.Client to use ActivitySource grpc/grpc-dotnet#2244

Merged

vitek-karas deleted the RemoveLinqExp branch August 16, 2023 14:40

	#if !NET7_0_OR_GREATER
	ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
	ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);
	#endif

	if (!IsNet7OrGreater)
	{
	ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
	ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);
	}

	if (activity.IsAllDataRequested)
	{
	ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
	ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);

[AOT] Remove usage of Linq.Expressions and especially lambda compilation #4695

[AOT] Remove usage of Linq.Expressions and especially lambda compilation #4695

Uh oh!

Conversation

vitek-karas commented Jul 26, 2023

Merge requirement checklist

Uh oh!

eerhardt Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

vitek-karas Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

vitek-karas Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

tarekgh Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

tarekgh Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

JamesNK Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

utpilla left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

vitek-karas commented Jul 28, 2023

Uh oh!

eerhardt commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitek-karas commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vitek-karas Jul 26, 2023 •

edited

Loading

tarekgh Jul 27, 2023 •

edited

Loading

eerhardt Jul 27, 2023 •

edited

Loading

JamesNK Jul 28, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

eerhardt commented Jul 28, 2023 •

edited

Loading

vitek-karas commented Jul 28, 2023 •

edited

Loading