Instrumenation.AWSLambda: Drop netstandard2, lose Newtonsoft (non-)dependency #1270
Conversation
|
BTW, it seems we are using an outdated tool dotnet-format instead of the built-in dotnet format subcommand in CI. The former seems to be completely confused by raw string literals. |
| @@ -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.
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.
|
One unrelated build failure, added to #716 (comment) |
| @@ -1,7 +1,7 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
| <TargetFramework>netstandard2.0</TargetFramework> | |||
| <TargetFramework>netcoreapp3.1</TargetFramework> | |||
There was a problem hiding this comment.
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.
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.
As the minimum .NET version in AWS Lambda is already 6.0, no compatibility problems are expected.
@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.
.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.
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.
.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.
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.
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.
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.
Lets do opposite.
- Create PR with .NET 6 bump.
- As a follow up, fix this issue.
There was a problem hiding this comment.
I'd prefer:
- Merge a PR without a bump to net 6 (this or Instrumentation.AWSLambda: Upgrade & explicitly depend on Newtonsoft.Json #1273)
- Publish a release
- .NET 6 bump
utpilla
added
the
comp:instrumentation.awslambda
Closes #1273 (alternative to that PR)
Related to a similar change in the XRay dependency: #1092
Changes
Quoting the CHANGELOG:
The minimum required .NET version is now .NET Core 3.1 (previously any netstandard2.0-compliant).
As the minimum .NET version in AWS Lambda is already 6.0, no compatibility problems are expected.
This resolves a warning that some dependency analyzers may produce where this package would transitively
depend on a vulnerable version of
Newtonsoft.JsonthroughAmazon.Lambda.APIGatewayEvents.
This also avoids a potential issue where the instrumentation would try to call a Newtonsoft.Json function
when no other package nor the app itself depends on Newtonsoft.Json, since the transitive dependency would be
ignored unless using application were compiled against a TargetFramework older than Core 3.1.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes[ ] Design discussion issue #N/A[ ] Changes in public API reviewedAPI remains identical