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

feat(propagation-utils): end pub-sub process span on promise settled #1055

Merged
merged 13 commits into from
Sep 24, 2022

Conversation

nordfjord
Copy link
Contributor

@nordfjord nordfjord commented Jun 11, 2022

When promises are returned from a mapping function we now
end the span when that promises resolves rather than doing
so immediately.

This makes sure that the processing span wraps the entirety of the
processing

Which problem is this PR solving?

  • The pubsub propagation module would end spans prematurely when promises were involved.

An example of what this looks like:
Screen Shot 2022-06-11 at 2 17 00 PM

Short description of the changes

  • Handle promises from mapping functions

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

@nordfjord nordfjord requested a review from a team June 11, 2022 18:14
@blumamir
Copy link
Member

@nordfjord thank you for the contribution :)
can you please run npm run lint:fix to revert all the linter changes? It will really help to focus on the logical changes while reviewing and make CI happy.

Also please add relevant unit tests for this change. Either in the propagation-utils package or in the aws-sdk instrumentation package which is consuming these utils. Thank you!

@blumamir blumamir changed the title Handle promises in pubsub propagation feat(propagation-utils): end pub-sub process span on promise settled Jun 12, 2022
@nordfjord
Copy link
Contributor Author

Thanks for taking the time to review this. I think this is ready for another review now.

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM.
After tests are green this can be merged AFAIC

Thank you for contributing this fix

When promises are returned from a mapping function we now
end the span when that promises resolves rather than doing
so immediately.

This makes sure that the processing span wraps the entirety of the
processing
@nordfjord
Copy link
Contributor Author

Not quite sure what was going on but I had to remove the --require @opentelemetry/contrib-test-utils part from the package.json test script so that the tests would pass.

I've verified that yarn test works locally now.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2022

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Sep 5, 2022
@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #1055 (d44c11a) into main (8873057) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1055      +/-   ##
==========================================
+ Coverage   96.08%   96.20%   +0.12%     
==========================================
  Files          14       26      +12     
  Lines         893     1529     +636     
  Branches      191      325     +134     
==========================================
+ Hits          858     1471     +613     
- Misses         35       58      +23     
Impacted Files Coverage Δ
...emetry-propagation-utils/src/pubsub-propagation.ts 88.75% <100.00%> (ø)
...emetry-instrumentation-aws-sdk/src/services/sqs.ts 100.00% <0.00%> (ø)
...ntation-aws-sdk/src/services/ServicesExtensions.ts 100.00% <0.00%> (ø)
...opentelemetry-instrumentation-aws-sdk/src/utils.ts 97.29% <0.00%> (ø)
...entation-aws-sdk/src/services/MessageAttributes.ts 91.89% <0.00%> (ø)
...emetry-instrumentation-aws-sdk/src/services/sns.ts 93.75% <0.00%> (ø)
...etry-instrumentation-aws-sdk/src/services/index.ts 100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.03% <0.00%> (ø)
...try-instrumentation-aws-sdk/src/services/lambda.ts 97.77% <0.00%> (ø)
... and 3 more

@rauno56
Copy link
Member

rauno56 commented Sep 13, 2022

catch {} is not supported in node 8. Will merge this after #1065.

@vmarchaud vmarchaud merged commit cb83d30 into open-telemetry:main Sep 24, 2022
@dyladan dyladan mentioned this pull request Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants