Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from Jul 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5120526
[AOT] Remove usage of Linq.Expressions and especially lambda compilation
vitek-karas Jul 26, 2023
1fdd8ee
Update src/OpenTelemetry/Trace/ExceptionProcessor.cs
utpilla Jul 28, 2023
d041885
Update src/OpenTelemetry/Trace/ExceptionProcessor.cs
utpilla Jul 28, 2023
132003d
Merge branch 'main' into RemoveLinqExp
utpilla Jul 28, 2023
0a2a105
Use the API directly even on netfx
vitek-karas Jul 28, 2023
4985d8d
Merge branch 'main' into RemoveLinqExp
vitek-karas Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/OpenTelemetry/Trace/ExceptionProcessor.cs
View file
Open in desktop
Expand Up @@ -17,9 +17,11 @@
#nullable enable

using System.Diagnostics;
using System.Runtime.InteropServices;
#if !NET6_0_OR_GREATER && !NETFRAMEWORK
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.InteropServices;
#endif

namespace OpenTelemetry.Trace;

Expand All @@ -31,6 +33,11 @@ internal sealed class ExceptionProcessor : BaseProcessor<Activity>

public ExceptionProcessor()
{
#if NET6_0_OR_GREATER || NETFRAMEWORK
this.fnGetExceptionPointers = Marshal.GetExceptionPointers;
#else
// When running on netstandard or similar the Marshal class is not a part of the netstandard API
// but it would still most likely be available in the underlying framework, so use reflection to retrieve it.
try
{
var flags = BindingFlags.Static | BindingFlags.Public;
Expand All @@ -43,6 +50,7 @@ public ExceptionProcessor()
{
throw new NotSupportedException($"'{typeof(Marshal).FullName}.GetExceptionPointers' is not supported", ex);
}
#endif
}

/// <inheritdoc />
Expand Down
16 changes: 4 additions & 12 deletions src/Shared/ActivityInstrumentationHelper.cs
View file
Open in desktop
Expand Up @@ -15,8 +15,6 @@
// </copyright>

using System.Diagnostics;
using System.Linq.Expressions;
using System.Reflection;
#pragma warning restore IDE0005

namespace OpenTelemetry.Instrumentation;
Expand All @@ -28,19 +26,13 @@ internal static class ActivityInstrumentationHelper

private static Action<Activity, ActivitySource> CreateActivitySourceSetter()
{
ParameterExpression instance = Expression.Parameter(typeof(Activity), "instance");
ParameterExpression propertyValue = Expression.Parameter(typeof(ActivitySource), "propertyValue");
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@noahfalk as an "fyi".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @cijothomas @reyang

Copy link
Contributor

@tarekgh tarekgh Jul 27, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@eerhardt eerhardt Jul 27, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@JamesNK JamesNK Jul 28, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: grpc/grpc-dotnet#2244

.SetMethod.CreateDelegate(typeof(Action<Activity, ActivitySource>));
}

private static Action<Activity, ActivityKind> CreateActivityKindSetter()
{
ParameterExpression instance = Expression.Parameter(typeof(Activity), "instance");
ParameterExpression propertyValue = Expression.Parameter(typeof(ActivityKind), "propertyValue");
PropertyInfo kindPropertyInfo = typeof(Activity).GetProperty("Kind");
var body = Expression.Assign(Expression.Property(instance, kindPropertyInfo), propertyValue);
return Expression.Lambda<Action<Activity, ActivityKind>>(body, instance, propertyValue).Compile();
return (Action<Activity, ActivityKind>)typeof(Activity).GetProperty("Kind")
.SetMethod.CreateDelegate(typeof(Action<Activity, ActivityKind>));
}
}