[Instrumentation.AWSLambda] Fix AoT warnings

[martincostello]

• Fixes Getting rid of Newtonsoft.Json dependency in OpenTelemetry.Instrumentation.AWSLambda #1523.
• Depends on [Instrumentation.AWSLambda] Update to net6.0 #1545.

Changes

Make OpenTelemetry.Instrumentation.AWSLambda AoT compatible by:

• Using System.Text.Json to deserialize SNS messages
• Updating to Amazon.Lambda packages that support native AoT.

• Appropriate CHANGELOG.md updated for non-trivial changes

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (71655ce) 73.91% compared to head (227b4ee) 80.70%.
Report is 123 commits behind head on main.

❗ Current head 227b4ee differs from pull request most recent head fad055b. Consider uploading reports for the commit fad055b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1544      +/-   ##
==========================================
+ Coverage   73.91%   80.70%   +6.78%     
==========================================
  Files         267      113     -154     
  Lines        9615     3073    -6542     
==========================================
- Hits         7107     2480    -4627     
+ Misses       2508      593    -1915     

Flag	Coverage Δ
unittests-Solution	80.70% <88.57%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...Telemetry.Exporter.InfluxDB/InfluxDBEventSource.cs	60.00% <ø> (ø)
...ry.Exporter.InfluxDB/InfluxDBExporterExtensions.cs	100.00% <100.00%> (ø)
...metry.Exporter.InfluxDB/InfluxDBMetricsExporter.cs	84.61% <ø> (-3.85%)	⬇️
...Telemetry.Exporter.InfluxDB/PointDataExtensions.cs	87.50% <100.00%> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV1.cs	100.00% <ø> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV2.cs	100.00% <ø> (ø)
...stana/Implementation/InstanaExporterEventSource.cs	0.00% <ø> (ø)
...try.Exporter.Instana/Implementation/InstanaSpan.cs	100.00% <ø> (ø)
...orter.Instana/Implementation/InstanaSpanFactory.cs	100.00% <ø> (ø)
...er.Instana/Implementation/InstanaSpanSerializer.cs	93.79% <ø> (ø)
... and 31 more

... and 225 files with indirect coverage changes

[martincostello]

@normj Do you have any insight into the deserialization of the payload here being right in the context of the changes here to use the source generator?

[Oberon00]

@martincostello There is a unit test with an example payload here:

opentelemetry-dotnet-contrib/test/OpenTelemetry.Instrumentation.AWSLambda.Tests/Implementation/AWSMessagingUtilsTests.cs

Lines 62 to 101 in 8a89a69

	[Fact]
	public void ExtractParentContext_SetParentFromMessageBatchIsEnabled_ParentIsSetFromSnsMessage()
	{
	AWSMessagingUtils.SetParentFromMessageBatch = true;
	var sqsEvent = new SQSEvent
	{
	Records = new List<SQSMessage>
	{
	new SQSMessage
	{
	MessageAttributes = new(),
	#pragma warning disable format // dotnet-format butchers the raw string & all following code (use dotnet format instead?)
	Body = /*lang=json,strict*/ """
	{
	"Type" : "Notification",
	"MessageId" : "f91f7f8e-77cc-51e7-ad08-231055044066",
	"TopicArn" : "arn:aws:sqs:us-east-1:123456789012:foo-bar-test-queue",
	"Subject" : "testsub",
	"Message" : "{\"This JSON string\": \"is in the SNS body\"}",
	"Timestamp" : "2023-03-29T11:27:04.056Z",
	"SignatureVersion" : "1",
	"Signature" : "base64string/redacted",
	"SigningCertURL" : "https://sns.us-east-1.amazonaws.com/SimpleNotificationService-1234567abc123def1234567890123467.pem",
	"UnsubscribeURL" : "https://sns.us-east-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sqs:us-east-1:123456789012:foo-bar-test-queue:123abcde-1234-1abc-1234-123456abcdef",
	"MessageAttributes" : {
	"traceparent" : {"Type":"String","Value":"00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-00"}
	}
	}
	""",
	},
	},
	};
	(PropagationContext parentContext, IEnumerable<ActivityLink>? links) = AWSMessagingUtils.ExtractParentContext(sqsEvent);
	Assert.NotEqual(default, parentContext);
	Assert.Equal(SpanId1, parentContext.ActivityContext.SpanId.ToHexString());
	Assert.Single(links!);
	}

If that test passes, the deserialization should actually be good, but feel free to double-check

[normj]

@normj Do you have any insight into the deserialization of the payload here being right in the context of the changes here to use the source generator?

Sorry I'm not understanding the question. Are you asking if the SourceGenerationContext is setup correctly for deserializing the SNSEvent.SNSMessage type correctly? If so yes that looks right for using source generator serializer.

[martincostello]

Yep, that was the question.

Thanks for the feedback both, I'll address these today.

[martincostello]

Have rebased onto #1545 and will rebase onto main once that is merged.

Just the native AoT changes can be reviewed here.

[Oberon00]

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?)

[martincostello]

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?

[Oberon00]

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.

[martincostello]

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.

[Oberon00]

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)

[martincostello]

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 😅.

[normj]

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.

[martincostello]

AoT failure will be resolved when #1540 is merged.

Cherry-picked the fix from 5fae692.

[normj]

As a non-component owner the changes look good.

[birojnayak]

good to see this is closed ...