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.

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

[aws] Combine Extensions.AWS, Instrumentation.AWS, & Instrumentation.AWSLambda into a single tag #2203

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

CodeBlanch
Copy link
Member

Changes

  • Use the same tag prefix (MinVerTagPrefix) for OpenTelemetry.Extensions.AWS, OpenTelemetry.Instrumentation.AWS, & OpenTelemetry.Instrumentation.AWSLambda.

  • Add STJ reference at 6.0.10 / 8.0.5 for OpenTelemetry.Instrumentation.AWSLambda since it uses the source generator.

Details

@srprash @ppittle @Oberon00 @rypdal

My goal here is to resolve the last build breaks due to vulnerable STJ package versions:

OpenTelemetry.Instrumentation.AWSLambda.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 6.0.0 has a known high severity vulnerability
OpenTelemetry.Instrumentation.AWS.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 6.0.0 has a known high severity vulnerability

The reason this is happening is OpenTelemetry.Instrumentation.AWS and OpenTelemetry.Instrumentation.AWSLambda reference OpenTelemetry.Extensions.AWS through NuGet, they don't use ProjectReference.

There isn't anything necessarily wrong with that, but it is a code smell IMO. Tightly coupled projects should be released together.

Examples:

What this PR is doing is using the same MinVerTagPrefix for OpenTelemetry.Extensions.AWS, OpenTelemetry.Instrumentation.AWS, & OpenTelemetry.Instrumentation.AWSLambda so they will always be versioned and released together.

We could just put out a NuGet for OpenTelemetry.Extensions.AWS to resolve the issue but I feel this is better design and overall better for users. Less chance for mismatched versions and conflicts for the coupled projects and their dependencies.

Please approve this PR if this works for AWS group!

PS: We could version all of the AWS packages together. Might simplify the versioning/upgrade story for everything. But I only did the coupled ones here. Happy to do them all if that is something AWS group wants.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)

… generator not present for netstandard2.0 builds of Lambda.
@CodeBlanch CodeBlanch requested a review from a team as a code owner October 10, 2024 17:09
@github-actions github-actions bot added comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda labels Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.28%. Comparing base (71655ce) to head (2a5f80b).
Report is 531 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2203       +/-   ##
===========================================
+ Coverage   73.91%   85.28%   +11.36%     
===========================================
  Files         267       35      -232     
  Lines        9615      761     -8854     
===========================================
- Hits         7107      649     -6458     
+ Misses       2508      112     -2396     
Flag Coverage Δ
unittests-Extensions 88.63% <ø> (?)
unittests-Extensions.AWS 83.41% <ø> (?)
unittests-Instrumentation.AWS 66.66% <ø> (?)

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

see 278 files with indirect coverage changes

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

LGTM. I would wait for some feedback from component owners before merge.

@ppittle
Copy link
Member

ppittle commented Oct 11, 2024

Given our recent discussion in the SIG about release strategy, I agree this change make sense.

I'd still like to review the other AWS packages - it may yet make sense for those to release separately.

Thanks for the change and write up!

@Kielek Kielek merged commit b3ac2b8 into open-telemetry:main Oct 11, 2024
61 checks passed
@CodeBlanch CodeBlanch deleted the aws-combine-releases branch October 11, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants