Skip to content

Conversation

@ppittle
Copy link
Member

@ppittle ppittle commented Mar 10, 2025

Fixes # #2652
Design discussion issue #

Changes

Set initial AWS Semantic Convention list capacity to guard against list resizing. This should have a very small performance improvement and help in concurrent unit tests.

Note: this code is not originally intended to be thread safe:

// No parallel invocation of the same lambda handler expected.
var functionTags = new AWSLambdaUtils(AWSSemanticConventions).GetFunctionTags(input, context, isColdStart);

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@ppittle
Copy link
Member Author

ppittle commented Mar 10, 2025

@martincostello - Would you be able to help test if this fixes your test in #2652 ?

Given that this is a concurrency issue, I'm not quite sure how to (repro) and test. But given the limited nature, I'm ok merging as-is.

@github-actions github-actions bot added comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda comp:resources.aws Things related to OpenTelemetry.Resources.AWS labels Mar 10, 2025
@martincostello
Copy link
Member

@ppittle I can take a look at using it tomorrow - like you say given it's concurrency-related, it's probably going to just be a case of "run it a bunch of times and check nothing breaks".

@ppittle
Copy link
Member Author

ppittle commented Mar 10, 2025

@ppittle I can take a look at using it tomorrow - like you say given it's concurrency-related, it's probably going to just be a case of "run it a bunch of times and check nothing breaks".

It'd be nice to confirm if it fixes your tests at least.

If testing this PR is too much trouble, we can work to merge the PR as it is - it does have a tiny perf benefit anyway.

@ppittle ppittle marked this pull request as ready for review March 10, 2025 21:05
@ppittle ppittle requested a review from a team as a code owner March 10, 2025 21:05
@martincostello
Copy link
Member

It was the only time I've seen that specific failure mode, but it's not too much trouble to test as long as there's a build of the NuGet packages in the CI I can grab. Then I can just open a draft PR to use it and run it 10-15 times and check there's no failures.

@martincostello
Copy link
Member

@ppittle Looks like the CI doesn't produce any artifacts that I can grab the .nupkg files from.

@ppittle
Copy link
Member Author

ppittle commented Mar 11, 2025

@ppittle Looks like the CI doesn't produce any artifacts that I can grab the .nupkg files from.

Ya, I looked around as well and didn't see any build artifacts published as part of PR.

@Kielek or @CodeBlanch - do you know if there is an established process to get a test build from a PR like this so @martincostello can test?

If not, I'm inclined to merge this PR and request an official beta release.

@Kielek
Copy link
Member

Kielek commented Mar 12, 2025

@ppittle, packages are build, but not exposes as artifacts during standard CI build.
What you can do is build your own copy of the nuget by mimic steps from

- name: dotnet restore ${{ steps.resolve-project.outputs.title }}
run: dotnet restore ${{ steps.resolve-project.outputs.project }}
- name: dotnet build ${{ steps.resolve-project.outputs.title }}
run: dotnet build ${{ steps.resolve-project.outputs.project }} --configuration Release --no-restore -p:Deterministic=true -p:BuildNumber=${{ github.run_number }}
- name: dotnet test ${{ steps.resolve-project.outputs.title }}
run: dotnet test ${{ steps.resolve-project.outputs.project }} --configuration Release --no-restore --no-build
- name: dotnet pack ${{ steps.resolve-project.outputs.title }}
run: dotnet pack ${{ steps.resolve-project.outputs.project }} --configuration Release --no-restore --no-build -p:PackTag=${{ github.ref_type == 'tag' && github.ref_name || '' }}

or just any other way calling build pack. There is no magic on the CI.
Alternative option - extend CI steps to store nuget-packages as artifacts, but it is rarely needed, so if your local build is sufficient I will go with this way.

@martincostello
Copy link
Member

I would recommend the CI being extended to upload the packages as artifacts, then it's trivial for people to download them and use them as-is.

Having them need to clone and compile everything first, not so much... 😄

@ppittle
Copy link
Member Author

ppittle commented Mar 12, 2025

@martincostello - I've built the AWSLambda nuget locally, can you give this a try:

NOTE: change the extension to .nupkg - I had to change it to a zip file to get around GitHubs file upload rules.

OpenTelemetry.Instrumentation.AWSLambda.1.11.2-alpha.0.8.nupkg.zip

@martincostello
Copy link
Member

@ppittle Looks like I need its dependencies too to be able to build it:

    C:\Coding\martincostello\alexa-london-travel\test\LondonTravel.Skill.AppHostTests\LondonTravel.Skill.AppHostTests.csproj : error NU1102:
      Unable to find package OpenTelemetry.Extensions.AWS with version (>= 1.11.2-alpha.0.8)
        - Found 9 version(s) in NuGet [ Nearest version: 1.11.1 ]
        - Versions from C:\Program Files\dotnet\library-packs were not considered
        - Versions from local were not considered

@martincostello
Copy link
Member

Alternative option - extend CI steps to store nuget-packages as artifacts

#2658

@ppittle
Copy link
Member Author

ppittle commented Mar 13, 2025

@ppittle Looks like I need its dependencies too to be able to build it:

    C:\Coding\martincostello\alexa-london-travel\test\LondonTravel.Skill.AppHostTests\LondonTravel.Skill.AppHostTests.csproj : error NU1102:
      Unable to find package OpenTelemetry.Extensions.AWS with version (>= 1.11.2-alpha.0.8)
        - Found 9 version(s) in NuGet [ Nearest version: 1.11.1 ]
        - Versions from C:\Program Files\dotnet\library-packs were not considered
        - Versions from local were not considered

Sorry about that - Here's both packages:

I checked the nuspec files and I don't see any other non-public transitive packages, so you should be good to go with just these two.

@martincostello
Copy link
Member

Re-ran the tests using the provided .nupkg files 12 times, and didn't observe the error reported in #2652: https://github.com/martincostello/alexa-london-travel/actions/runs/13844181275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda comp:resources.aws Things related to OpenTelemetry.Resources.AWS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants