search

open-telemetry / opentelemetry-dotnet-contrib

This repository contains set of components extending functionality of the OpenTelemetry .NET SDK. Instrumentation libraries, exporters, and other components can find their home here.
https://opentelemetry.io
Apache License 2.0
448 stars 273 forks source link

Make `OpenTelemetry.Instrumentation.StackExchangeRedis` package AOT safe. #1341

Closed eerhardt closed 10 months ago

eerhardt commented 1 year ago

Issue with OpenTelemetry.Instrumentation.StackExchangeRedis

Related to: https://github.com/open-telemetry/opentelemetry-dotnet/issues/3429 cc: @vitek-karas, @Yun-Ting, @reyang

Runtime version: net6.0, net7.0.

Is this a feature request or a bug?

Additional Context

Here are the warnings I see when adding this library to the OpenTelemetry.AotCompatibility.TestApp.csproj app:

C:\git\opentelemetry-dotnet-contrib\src\Shared\ActivityInstrumentationHelper.cs(36): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.ActivityInstrumentationHelper.CreateActivitySourceSetter(): Using member 'System.Linq.Expressions.Expression.Property(Expression,String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\Shared\ActivityInstrumentationHelper.cs(44): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.ActivityInstrumentationHelper.CreateActivityKindSetter(): Using member 'System.Linq.Expressions.Expression.Property(Expression,String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\Shared\MultiTypePropertyFetcher.cs(64): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.MultiTypePropertyFetcher`1.Fetch(Object): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Reflection.TypeInfo.DeclaredProperties.get'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\Shared\MultiTypePropertyFetcher.cs(67): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.MultiTypePropertyFetcher`1.Fetch(Object): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Type.GetProperty(String,BindingFlags)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\Shared\PropertyFetcher.cs(79): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.PropertyFetcher`1.TryFetch(Object,!0&): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Reflection.TypeInfo.DeclaredProperties.get'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\Shared\PropertyFetcher.cs(82): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.PropertyFetcher`1.TryFetch(Object,!0&): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(191): Trim analysis warning IL2070: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.CreateFieldGetter<TField>(Type,String,BindingFlags): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.NonPublicFields' in call to 'System.Type.GetField(String,BindingFlags)'. The parameter 'classType' of method 'OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.CreateFieldGetter<TField>(Type,String,BindingFlags)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(195): AOT analysis warning IL3050: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.CreateFieldGetter<TField>(Type,String,BindingFlags): Using member 'System.Reflection.Emit.DynamicMethod.DynamicMethod(String,Type,Type[],Boolean)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. Creating a DynamicMethod requires dynamic code. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\Shared\MultiTypePropertyFetcher.cs(99): AOT analysis warning IL3050: OpenTelemetry.Instrumentation.MultiTypePropertyFetcher`1.PropertyFetch.FetcherForProperty(PropertyInfo): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\Shared\PropertyFetcher.cs(107): AOT analysis warning IL3050: OpenTelemetry.Instrumentation.PropertyFetcher`1.PropertyFetch.FetcherForProperty(PropertyInfo): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(31): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.<>c.<.cctor>b__4_0(): Using member 'System.Reflection.Assembly.GetType(String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\git\opentelemetry-dotnet-contrib\src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(32): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.<>c.<.cctor>b__4_0(): Using member 'System.Reflection.Assembly.GetType(String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/ArrayOperations.cs(23): AOT analysis warning IL3050: System.Linq.Expressions.Interpreter.NewArrayInitInstruction.Run(InterpretedFrame): Using member 'System.Array.CreateInstance(Type,Int32)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/ArrayOperations.cs(52): AOT analysis warning IL3050: System.Linq.Expressions.Interpreter.NewArrayInstruction.Run(InterpretedFrame): Using member 'System.Array.CreateInstance(Type,Int32)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/ArrayOperations.cs(87): AOT analysis warning IL3050: System.Linq.Expressions.Interpreter.NewArrayBoundsInstruction.Run(InterpretedFrame): Using member 'System.Array.CreateInstance(Type,Int32[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs(28): AOT analysis warning IL3050: System.Dynamic.Utils.TypeUtils.GetNullableType(Type): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/NewArrayExpression.cs(228): AOT analysis warning IL3050: System.Linq.Expressions.Expression.NewArrayBounds(Type,IEnumerable`1<Expression>): Using member 'System.Type.MakeArrayType()' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/NewArrayExpression.cs(232): AOT analysis warning IL3050: System.Linq.Expressions.Expression.NewArrayBounds(Type,IEnumerable`1<Expression>): Using member 'System.Type.MakeArrayType(Int32)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/NewArrayExpression.cs(172): AOT analysis warning IL3050: System.Linq.Expressions.Expression.NewArrayInit(Type,IEnumerable`1<Expression>): Using member 'System.Type.MakeArrayType()' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/BinaryExpression.cs(2239): AOT analysis warning IL3050: System.Linq.Expressions.Expression.GetResultTypeOfShift(Type,Type): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
/_/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/MethodCallExpression.cs(1379): AOT analysis warning IL3050: System.Linq.Expressions.Expression.ApplyTypeArgs(MethodInfo,Type[]): Using member 'System.Reflection.MethodInfo.MakeGenericMethod(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\git\opentelemetry-dotnet-contrib\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
vitek-karas commented 1 year ago

I did some analysis on how to fix these (note that I used .NET 8 SDK and retargeted the test project to net8, but it doesn't seem to matter much). It all seems very fixable:

Field getters

Accessing the field on an internal type

The activity converter uses reflection to access fields on the command and message types.

src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(31): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.<>c.<.cctor>b__4_0(): Using member 'System.Reflection.Assembly.GetType(String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed.
src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(32): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.<>c.<.cctor>b__4_0(): Using member 'System.Reflection.Assembly.GetType(String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed.

The types involved in this seem to be known. The code gets the assembly of a known Redis type and then uses Assembly.GetType("") to query for the internal type of the command and message. It should be possible to replace these with Type.GetType("TypeName, StackExchange.Redis") which is a pattern the trimmer understands. It's highly unlikely the assembly name will change or the location of the classes will change. With that change the two warnings above will be fixed.

Fast field getter

The implementation is a helper which uses Ref.Emit to generate a fast field getter from an internal type.

src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(191): Trim analysis warning IL2070: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.CreateFieldGetter<TField>(Type,String,BindingFlags): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.NonPublicFields' in call to 'System.Type.GetField(String,BindingFlags)'. The parameter 'classType' of method 'OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.CreateFieldGetter<TField>(Type,String,BindingFlags)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
src\OpenTelemetry.Instrumentation.StackExchangeRedis\Implementation\RedisProfilerEntryToActivityConverter.cs(195): AOT analysis warning IL3050: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.CreateFieldGetter<TField>(Type,String,BindingFlags): Using member 'System.Reflection.Emit.DynamicMethod.DynamicMethod(String,Type,Type[],Boolean)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. Creating a DynamicMethod requires dynamic code.

The first warning could be fixed by adding DynamicallyAccessedMember annotation onto the classType parameter and with the above change to use Type.GetType the trimmer will be able to correctly preserve the fields as needed.

The second warning is AOT only, reflection emit is not supported in AOT. The only current solution is to fall back to a less performant implementation which calls FieldInfo.GetValue every time.

In the future this would be a great candidate to use UnsafeAccessor attributes for, but it would require to also have UnsafeAccessorType which doesn't exist yet.

Ultimately StackExchange.Redis should switch to use public types for the event payloads and all reflection could be removed. TODO: File an issue in the redis repo for this.

PropertyFetcher

The class is almost a verbatim copy of PropertyFetcher from open-telemetry repo. This has been recently made AOT compatible, so just updating the copy should fix the following warnings:

src\Shared\PropertyFetcher.cs(79): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.PropertyFetcher`1.TryFetch(Object,!0&): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Reflection.TypeInfo.DeclaredProperties.get'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
src\Shared\PropertyFetcher.cs(82): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.PropertyFetcher`1.TryFetch(Object,!0&): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
src\Shared\PropertyFetcher.cs(107): AOT analysis warning IL3050: OpenTelemetry.Instrumentation.PropertyFetcher`1.PropertyFetch.FetcherForProperty(PropertyInfo): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.

Note that this would introduce new warning in each place where PropertyFetcher is used. There's only one place in the Redis instrumentation. This needs to be fixed by making sure that the CommandAndKey property is preserved on the Message type from Redis. This could potentially be fixed by replacing a PropertyFetcher here with direct reflection implementation, or extending PropertyFetcher to accept PropertyInfo instead of a name, which would make that code path trim compatible.

ActivityInstrumentationHelper

The class is a verbatim copy of the ActivityInstrumentationHelper from open-telemetry which has been fixed for AOT compatibility. Just updating the copy should fix these warnings:

src\Shared\ActivityInstrumentationHelper.cs(36): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.ActivityInstrumentationHelper.CreateActivitySourceSetter(): Using member 'System.Linq.Expressions.Expression.Property(Expression,String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed.
src\Shared\ActivityInstrumentationHelper.cs(44): Trim analysis warning IL2026: OpenTelemetry.Instrumentation.ActivityInstrumentationHelper.CreateActivityKindSetter(): Using member 'System.Linq.Expressions.Expression.Property(Expression,String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed.

It also fixes all the 6 warnings from the System.Linq.Expressions assembly as this is the only place where Linq.Expressions is used.

MultiTypePropertyFetcher

The class is basically a PropertyFetcher which can work on multiple different payload types at the same time. It could be reimplemented either by making the same changes as PropertyFetcher went through to make it AOT compatible (see above), or by being a simple dictionary over multiple PropertyFetcher instances.

In any case, this is not used by the Redis instrumentation library, it's only compiled in because all of the shared helper classes are compiled into it. In reality it's only used by the Elasticsearch instrumentation library. So an easier fix would be to remove it from the library completely (reducing its size along the way).

src\Shared\MultiTypePropertyFetcher.cs(64): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.MultiTypePropertyFetcher`1.Fetch(Object): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Reflection.TypeInfo.DeclaredProperties.get'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
src\Shared\MultiTypePropertyFetcher.cs(67): Trim analysis warning IL2075: OpenTelemetry.Instrumentation.MultiTypePropertyFetcher`1.Fetch(Object): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Type.GetProperty(String,BindingFlags)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
src\Shared\MultiTypePropertyFetcher.cs(99): AOT analysis warning IL3050: OpenTelemetry.Instrumentation.MultiTypePropertyFetcher`1.PropertyFetch.FetcherForProperty(PropertyInfo): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime.