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

[Instrumentation.AWSLambda] Fix AoT warnings #1544

Merged
merged 2 commits into from
Jan 24, 2024
Merged

[Instrumentation.AWSLambda] Fix AoT warnings #1544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
187ac3c
[Instrumentation.AWSLambda] Fix AoT warnings
martincostello Jan 22, 2024
fad055b
Fix AoT test app
martincostello Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Instrumentation.AWSLambda/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
([#1295](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1295))
* BREAKING: Target `net6.0` instead of `netstandard2.0`
([#1545](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1545))
* Add support for native AoT.
`Amazon.Lambda.*` NuGet package dependencies have been upgraded, see package
dependencies for details.
([#1544](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1544))

## 1.2.0-beta.1

Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry.Instrumentation.AWSLambda/Implementation/AWSMessagingUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text.Json;
using Amazon.Lambda.SNSEvents;
using Amazon.Lambda.SQSEvents;
using Newtonsoft.Json;
using OpenTelemetry.Context.Propagation;

namespace OpenTelemetry.Instrumentation.AWSLambda.Implementation;
Expand Down Expand Up @@ -132,7 +132,7 @@ internal static PropagationContext ExtractParentContext(SNSEvent.SNSMessage? mes
{
try
{
snsMessage = JsonConvert.DeserializeObject<SNSEvent.SNSMessage>(body);
snsMessage = JsonSerializer.Deserialize(body, SourceGenerationContext.Default.SNSMessage);
}
catch (Exception)
{
Expand Down
9 changes: 4 additions & 5 deletions src/OpenTelemetry.Instrumentation.AWSLambda/OpenTelemetry.Instrumentation.AWSLambda.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Amazon.Lambda.APIGatewayEvents" Version="2.4.1" />
<PackageReference Include="Amazon.Lambda.Core" Version="1.2.0" />
<PackageReference Include="Amazon.Lambda.SNSEvents" Version="2.0.0" />
<PackageReference Include="Amazon.Lambda.SQSEvents" Version="2.1.0" />
<PackageReference Include="Amazon.Lambda.APIGatewayEvents" Version="2.7.0" />
<PackageReference Include="Amazon.Lambda.Core" Version="2.2.0" />
Copy link
Member

@Oberon00 Oberon00 Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is raising the lower bound of these dependencies necessary? In particular this one from major version 1 to 2? After all, these define which versions we can instrument. If so, please add a changelog entry (Sth like: Amazon Lambda package Versions below 2.1 no longer supported, see package dependencies for details)

(Or maybe this dependency was even already overridden by a transitive dependency through another Amazon.Lambda.* dependency where we were already using 2.x?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These versions all correlate to the versions that first introduced support for native AoT: CHANGELOG.

Version 2.1.0 of Amazon.Lambda.Core is the first version that added explicit support for net6.0, which was required by the changes asked for in #1545, and the only release since then was the 2.2.0 release for net8.0 support that resolves the native AoT warnings.

Based on that, I think these are reasonable increases to require of users while also making this assembly usable without trim warnings in native AoT applications. There were also already breaking changes listed in the CHANGELOG for this library before this PR or #1545.

Thoughts @normj?

Copy link
Member

@Oberon00 Oberon00 Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

while also making this assembly usable without trim warnings in native AoT applications

I wonder, because I assume the following:

  1. Users of the instrumentation should still have their own dependencies on Lambda packages and not rely on transitive dependencies here (unfortunately no implementation/api distinction exists in Nuget like in Java Gradle modules)
  2. If users want AoT or trimming, they can & should upgrade their own dependencies as needed. The library we release only specifies a lower bound.

Based on what you write with the .NET 6 support, an upgrade to 2.1 is totally reasonable without question, but I wonder about 2.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can downgrade it a bit in my native AoT Lambda using .NET 8 and see if I get any IL warnings (martincostello/alexa-london-travel#1020) or not.

My library does already specify its own dependencies for the Lambda assemblies it needs, but I don't use API Gateway, SNS or SQS, but I get all of those dependencies via this library transiently. Needing the end-user to manually upgrade the transient dependencies to get it to work properly for native AoT isn't ideal.

Copy link
Member

@Oberon00 Oberon00 Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

but I don't use API Gateway, SNS or SQS, but I get all of those dependencies via this library transiently

Good point. This is not ideal anyway. I don't think there is a way to have these as compile-only dependencies and then check at runtime if they are actually present? (But this of course goes way beyond the scope of this PR anyways)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Norm might know some tricks for dealing with that as the core AWS SDKs have to do similar things for things like STS authentication via the built-in fallback credential providers, but yeah that's a bit of a spiral out of the original scope here 😅.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I can't think of any special tricks here. The only difference between 2.1 and 2.2 for Amazon.Lambda.Core is updates to the project file to have the .NET 8 target and mark the package trimmable for .NET 8 if that makes you feel better taking the updated dependency.

<PackageReference Include="Amazon.Lambda.SNSEvents" Version="2.1.0" />
<PackageReference Include="Amazon.Lambda.SQSEvents" Version="2.2.0" />
<PackageReference Include="OpenTelemetry.Extensions.AWS" Version="1.3.0-beta.1" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />
</ItemGroup>

<ItemGroup>
Expand Down
20 changes: 20 additions & 0 deletions src/OpenTelemetry.Instrumentation.AWSLambda/SourceGenerationContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using System.Text.Json.Serialization;
using Amazon.Lambda.SNSEvents;

namespace OpenTelemetry.Instrumentation.AWSLambda;

/// <summary>
/// "Source Generation" is feature added to System.Text.Json in .NET 6.0.
/// This is a performance optimization that avoids runtime reflection when performing serialization.
/// Serialization metadata will be computed at compile-time and included in the assembly.
/// <see href="https://learn.microsoft.com/dotnet/standard/serialization/system-text-json/source-generation-modes" />.
/// <see href="https://learn.microsoft.com/dotnet/standard/serialization/system-text-json/source-generation" />.
/// <see href="https://devblogs.microsoft.com/dotnet/try-the-new-system-text-json-source-generator/" />.
/// </summary>
[JsonSerializable(typeof(SNSEvent.SNSMessage))]
martincostello marked this conversation as resolved.
Show resolved Hide resolved
internal sealed partial class SourceGenerationContext : JsonSerializerContext
{
}
4 changes: 2 additions & 2 deletions test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
.github\workflows\ci.yml so that it runs for the project being added.
-->
<TrimmerRootAssembly Include="OpenTelemetry.Extensions" />
<TrimmerRootAssembly Include="OpenTelemetry.Extensions.AWS" />
<TrimmerRootAssembly Include="OpenTelemetry.Extensions.Enrichment" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.Runtime" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AWSLambda" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.EventCounters" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.Runtime" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.StackExchangeRedis" />
<TrimmerRootAssembly Include="OpenTelemetry.ResourceDetectors.AWS" />
<TrimmerRootAssembly Include="OpenTelemetry.ResourceDetectors.Azure" />
Expand Down
Loading