-
Notifications
You must be signed in to change notification settings - Fork 289
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.
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
Instrumenation.AWSLambda: Drop netstandard2, lose Newtonsoft (non-)dependency #1270
Changes from all commits
910bf16
c942209
0fb3628
10063e3
5acd530
3f06162
052e4cc
d65f14c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,47 @@ public void ExtractParentContext_SetParentFromMessageBatchIsEnabled_ParentIsSetF | |
Assert.Equal(2, links.Count()); | ||
} | ||
|
||
[Fact] | ||
public void ExtractParentContext_SetParentFromMessageBatchIsEnabled_ParentIsSetFromSnsMessage() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this test, while generally IMHO useful, did not catch the error about the broken Newtonsoft dependency, because the MS test framework itself pulls in a Newtonsoft.Json assembly. Probably a integration test would be needed to see the issue, but since I never added one and wouldn't know how to do it, I skipped on that. |
||
{ | ||
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); | ||
} | ||
|
||
private static SQSEvent CreateSqsEventWithMessages(string[] spans) | ||
{ | ||
var @event = new SQSEvent { Records = new List<SQSMessage>() }; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change it to net6.0? netcoreapp3.1 is deprecated and might leads to issues in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It already passed CI though (except for a timeout in the coverage build).
I wouldn't want to switch right to 6. If netcoreapp3.1 is a problem, I'd stay on netstandard2.0 and add a conditional dependency on System.Text.Json just like in #1092
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Oberon00 Why do you not want to target net6.0, when the minimum runtime version required by ASWLambda is net6.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.NET 6 is not even 2 years old. And while the .NET 6 runtime is the oldest one supported by AWS, you should still be able compile your app against older versions and run them. Also, you could use a containerized runtime with an older version.
That's why I'd prefer not to use such a recent version as minimum, especially when it's only a bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created PR #1273 as an alternative to this, where I do not touch the .NET version at all, staying on netstandard2.0, adding an explicit dependency on Newtonsoft.Json instead.
Probably the .NET version upgrade should be done after the next release of the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.NET 6 is marked for LTS (long-term support) by .NET team and if AWS Lambda has chosen it as the minimum version, then it makes sense for its corresponding instrumentation library to do the same.
https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For AWS Lambda, is it possible for a consuming application to be targeting something lower than .NET 6? If not, then why would you be bothered by any of the lower and older targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@utpilla #1270 (comment):
To elaborate on the containerized runtime: https://docs.aws.amazon.com/lambda/latest/dg/images-create.html
I do think we should raise the requirement to net6 eventually, but not for a bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do opposite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer: