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

[BUG] NextLinkOperationImplementation uses AOT incompatible BinaryData APIs #44127

Closed
eerhardt opened this issue May 17, 2024 · 10 comments · Fixed by #44208
Closed

[BUG] NextLinkOperationImplementation uses AOT incompatible BinaryData APIs #44127

eerhardt opened this issue May 17, 2024 · 10 comments · Fixed by #44208
Assignees
@live1206
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@eerhardt
Copy link
Member

Library name and version

Azure.Monitor.OpenTelemetry.Exporter 1.3.0-beta.2

Describe the bug

NextLinkOperationImplementation is causing AOT warnings in the following code:

azure-sdk-for-net/sdk/core/Azure.Core/src/Shared/NextLinkOperationImplementation.cs

Line 225 in 34daa9b

var data = new BinaryData(new { version = RehydrationTokenVersion, id = operationId, requestMethod = requestMethod.ToString(), initialUri = startRequestUri.AbsoluteUri, nextRequestUri, headerSource, lastKnownLocation, finalStateVia });

azure-sdk-for-net/sdk/core/Azure.Core/src/Shared/NextLinkOperationImplementation.cs

Line 101 in 34daa9b

var lroDetails = ModelReaderWriter.Write(rehydrationToken!, ModelReaderWriterOptions.Json).ToObjectFromJson<Dictionary<string, string>>();

NOTE: if you use 1.3.0-beta.1 you don't get any warnings. This is new in 1.3.0-beta.2.

I believe the warnings were introduced in #42686.

@m-redding @TimothyMothra - do you know why these warnings aren't being caught by https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.AotCompatibilityTestApp/test-aot-compat.ps1?

Expected behavior

The above app shouldn't produce any AOT warnings. See also #35572 which fixed the warnings initially.

Actual behavior

It produces the following warnings:

ILC : Trim analysis warning IL2026: System.BinaryData.BinaryData(Object,JsonSerializerOptions,Type): Using member 'System.Text.Json.JsonSerializer.SerializeToUtf8Bytes(Object,Type,JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo orJsonSerializerContext, or make sure all of the required types are preserved. [C:\Users\eerhardt\source\repos\ConsoleApp123\ConsoleApp123\ConsoleApp123.csproj]
ILC : AOT analysis warning IL3050: System.BinaryData.BinaryData(Object,JsonSerializerOptions,Type): Using member 'System.Text.Json.JsonSerializer.SerializeToUtf8Bytes(Object,Type,JsonSerializerOptions)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications. [C:\Users\eerhardt\source\repos\ConsoleApp123\ConsoleApp123\ConsoleApp123.csproj]
ILC : Trim analysis warning IL2026: System.BinaryData.ToObjectFromJson<T>(JsonSerializerOptions): Using member 'System.Text.Json.JsonSerializer.Deserialize(ReadOnlySpan`1<Byte>,Type,JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved. [C:\Users\eerhardt\source\repos\ConsoleApp123\ConsoleApp123\ConsoleApp123.csproj]
ILC : AOT analysis warning IL3050: System.BinaryData.ToObjectFromJson<T>(JsonSerializerOptions): Using member 'System.Text.Json.JsonSerializer.Deserialize(ReadOnlySpan`1<Byte>,Type,JsonSerializerOptions)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications. [C:\Users\eerhardt\source\repos\ConsoleApp123\ConsoleApp123\ConsoleApp123.csproj]

Reproduction Steps

dotnet publish the following project

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishAot>true</PublishAot>
    <TrimmerSingleWarn>false</TrimmerSingleWarn>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Azure.Monitor.OpenTelemetry.Exporter" Version="1.3.0-beta.2" />
    <!-- Analyze the whole assembly -->
    <TrimmerRootAssembly Include="Azure.Monitor.OpenTelemetry.Exporter" />
  </ItemGroup>
</Project>

Environment

No response
@github-actions github-actions bot added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label May 17, 2024
@TimothyMothra
Copy link
Contributor

do you know why these warnings aren't being caught by https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.AotCompatibilityTestApp/test-aot-compat.ps1?

Hello, that test app isn't connected to any CI. I added that to manually verify we were AOT compliant for the NET8 release.

@m-redding added a CI pipeline in Dec #40629 and it's on my backlog to onboard to this. This was a lower priority because development has slowed on the Exporter. I hadn't considered that new changes in Azure.Core could break us.

@eerhardt
Copy link
Member Author

I hadn't considered that new changes in Azure.Core could break us.

Yes! Anything changing in your dependencies might break/affect your code. A lower level API could decide to do something incompatible and add a [RequiresUnreferencedCode] attribute to a method you call, and now you have warnings.

So it would be good to get CI checks to ensure libraries that should be AOT compatible, stay AOT compatible.

@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. Azure.Core and removed needs-triage This is a new issue that needs to be triaged to the appropriate team. labels May 18, 2024
@jsquire
Copy link
Member

jsquire commented May 18, 2024

@live1206: This is related to the LRO rehydration changes. Please take a look at ways we can make this AOT compatible, given that we can't know what libraries are using them.

//cc: @ArthurMa1978, @m-nash

@jsquire jsquire mentioned this issue May 18, 2024
Merged
@live1206 live1206 mentioned this issue May 19, 2024
Closed
@TimothyMothra
Copy link
Contributor

I'm having some issues onboarding to the AOT CI. I'll need to follow up with @m-redding when she's available.

Some other questions/concerns came up while onboarding.

  • Onboarding our Exporter to the AOT CI will only run in the monitor subdirectory. This would catch regressions in my library before releasing, but this wouldn't have caught changes as they occur in the Azure.Core library.
  • Azure.Core is already onboarded to the AOT CI (link) and it didn't catch this regression.

@m-redding
Copy link
Member

Sorry for the delay - was OOF. The AOT CI was passing for this particular PR, I will follow up on why it didn't catch this regression. I'm assuming it's because our pipelines are still running .NET 7 which is a known limitation of the CI.
And @TimothyMothra I'll respond to your email today about onboarding exporter (although like you mentioned, this wouldn't have prevented this).

@live1206 live1206 mentioned this issue May 23, 2024
Merged
@TimothyMothra TimothyMothra mentioned this issue May 30, 2024
Merged
@m-redding
Copy link
Member

After further investigation it turns out this was not a regression in Azure.Core. These warnings had already been baselined before I updated the .NET 8 pipeline. This is the previous version of the expected warnings from December:

azure-sdk-for-net/sdk/core/Azure.Core/tests/compatibility/ExpectedAotWarnings.txt

Lines 19 to 22 in c47ab4d

ILC : Trim analysis warning IL2026: System\.BinaryData\.BinaryData\(Object,JsonSerializerOptions,Type\): Using member 'System\.Text\.Json\.JsonSerializer\.SerializeToUtf8Bytes\(Object,Type,JsonSerializerOptions\)' which has 'RequiresUnreferencedCodeAttribute'
ILC : AOT analysis warning IL3050: System\.BinaryData\.BinaryData\(Object,JsonSerializerOptions,Type\): Using member 'System\.Text\.Json\.JsonSerializer\.SerializeToUtf8Bytes\(Object,Type,JsonSerializerOptions\)' which has 'RequiresDynamicCodeAttribute'
ILC : Trim analysis warning IL2026: System\.BinaryData\.ToObjectFromJson<T>\(JsonSerializerOptions\): Using member 'System\.Text\.Json\.JsonSerializer\.Deserialize\(ReadOnlySpan`1<Byte>,Type,JsonSerializerOptions\)' which has 'RequiresUnreferencedCodeAttribute'
ILC : AOT analysis warning IL3050: System\.BinaryData\.ToObjectFromJson<T>\(JsonSerializerOptions\): Using member 'System\.Text\.Json\.JsonSerializer\.Deserialize\(ReadOnlySpan`1<Byte>,Type,JsonSerializerOptions\)' which has 'RequiresDynamicCodeAttribute'

The changes in #42686 didn't introduce these. I'll work on getting them fixed regardless, and the pipelines are now updated to .NET 8 so they are more accurate, but I don't entirely understand how/why these warnings are impacting monitor exporter.

@m-redding
Copy link
Member

Ok I figured it out. These warnings are coming directly from System.Memory.Data because in Azure.Core, we have a reference on 1.0.2. If I update the reference to 8.0.0 then the warnings are resolved.

The downside is that we cannot upgrade to 8.0.0 because then we would have to upgrade STJ and a few other packages to 8 as well, which would impact our compatibility with Functions and/or Powershell.

At this point I think the best next steps would be to include a direct reference on System.Memory.Data 8.0.0 in monitor exporter

@jsquire
Copy link
Member

jsquire commented May 30, 2024

Thanks for digging in, @m-redding, and for figuring out the best path forward!

@live1206 live1206 reopened this May 31, 2024
@live1206
Copy link
Member

@m-redding Since we have further fix to resolve this issue, will you take over this issue instead?

@m-redding m-redding mentioned this issue May 31, 2024
Open
@m-redding
Copy link
Member

@live1206 this can be closed since there is no further action for Core. We already have a tracking issue for upgrading System.Memory.Data in #41941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@live1206 live1206

Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Milestone
No milestone
5 participants
@jsquire @live1206 @eerhardt @TimothyMothra @m-redding