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

fix(amqplib): use extracted context for message consuming #1354

Merged

Conversation

nerumo
Copy link
Contributor

@nerumo nerumo commented Jan 12, 2023

fixes #1351 baggage propagation

Which problem is this PR solving?

the baggage is lost while consuming messages since the extracted context isn't passed in

Short description of the changes

we pass in the extracted context to the context.with(....)

@blumamir
Copy link
Member

Thanks for the fix. Can you please add a test for that?

@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #1354 (0398eb6) into main (799b10b) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1354   +/-   ##
=======================================
  Coverage   96.08%   96.08%           
=======================================
  Files          14       14           
  Lines         893      893           
  Branches      191      191           
=======================================
  Hits          858      858           
  Misses         35       35           

@nerumo nerumo force-pushed the nerumo/amqplib-looses-baggage-1351 branch from 908cabb to 7db0a1f Compare January 12, 2023 11:23
@nerumo
Copy link
Contributor Author

nerumo commented Jan 12, 2023

Thanks for the fix. Can you please add a test for that?

I can't make the repository work on my machine (node-gyp native build problems). The scoped bootstrap is also failing. It's taking too much time for me, I can't continue here :(

I pushed the untested/incomplete test

@nerumo
Copy link
Contributor Author

nerumo commented Jan 12, 2023

so, got it running in WSL. and funny, the test I wrote worked on the first try 🎉

@nerumo nerumo marked this pull request as ready for review January 12, 2023 13:05
@nerumo nerumo requested a review from a team January 12, 2023 13:05
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.

Thanks!
Can you please fix the linting?

@nerumo
Copy link
Contributor Author

nerumo commented Jan 17, 2023

Thanks! Can you please fix the linting?

hmm...it's not the amqplib module that is failing. IMHO the linting does not fail because of my change

@Flarna
Copy link
Member

Flarna commented Jan 17, 2023

/__w/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/instrumentation-amqplib/test/amqplib-callbacks.test.ts
   17:9   error  Replace `expect` with `·expect·`                                                                                                                                                                       prettier/prettier
   18:9   error  Replace `AmqplibInstrumentation` with `·AmqplibInstrumentation·`                                                                                                                                       prettier/prettier
   31:9   error  Replace `Baggage,·context,·propagation,·SpanKind` with `·Baggage,·context,·propagation,·SpanKind·`                                                                                                     prettier/prettier
   32:9   error  Replace `asyncConfirmSend,·asyncConsume,·shouldTest` with `·asyncConfirmSend,·asyncConsume,·shouldTest·`                                                                                               prettier/prettier
   39:9   error  Replace `CompositePropagator,·W3CBaggagePropagator,·W3CTraceContextPropagator` with `⏎··CompositePropagator,⏎··W3CBaggagePropagator,⏎··W3CTraceContextPropagator,⏎`                                    prettier/prettier
   47:37  error  Insert `⏎······`                                                                                                                                                                                       prettier/prettier
   48:7   error  Replace `propagators:·[new·W3CBaggagePropagator(),·new·W3CTraceContextPropagator()` with `··propagators:·[⏎··········new·W3CBaggagePropagator(),⏎··········new·W3CTraceContextPropagator(),⏎········`  prettier/prettier
   49:1   error  Replace `····})` with `······})⏎····`                                                                                                                                                                  prettier/prettier
   77:38  error  Delete `⏎··········`                                                                                                                                                                                   prettier/prettier
   81:14  error  Replace `durable:·false` with `·durable:·false·`                                                                                                                                                       prettier/prettier
  100:16  error  Delete `⏎······`                                                                                                                                                                                       prettier/prettier
  135:1   error  Delete `··`                                                                                                                                                                                            prettier/prettier
  167:11  error  Delete `··`                                                                                                                                                                                            prettier/prettier
  198:53  error  Replace `context.active(),` with `⏎········context.active(),⏎·······`                                                                                                                                  prettier/prettier
  199:9   error  Replace `key1:·{value:·'value1'` with `··key1:·{·value:·'value1'·`                                                                                                                                     prettier/prettier
  200:7   error  Replace `})` with `··})⏎······`                                                                                                                                                                        prettier/prettier
  202:29  error  Replace `⏎··········queueName,⏎··········Buffer.from(msgPayload)⏎········` with `queueName,·Buffer.from(msgPayload)`                                                                                   prettier/prettier
  210:12  error  Insert `⏎············`                                                                                                                                                                                 prettier/prettier
  211:1   error  Insert `··`                                                                                                                                                                                            prettier/prettier
  212:11  error  Replace `}` with `··},⏎··········`                                                                                                                                                                     prettier/prettier
  256:45  error  Delete `⏎··········`                                                                                                                                                                                   prettier/prettier
  260:14  error  Replace `durable:·false` with `·durable:·false·`                                                                                                                                                       prettier/prettier
  279:16  error  Delete `⏎······`                                                                                                                                                                                       prettier/prettier
  306:13  error  Delete `··`                                                                                                                                                                                            prettier/prettier
  311:1   error  Delete `··`                                                                                                                                                                                            prettier/prettier
  319:1   error  Delete `··`                                                                                                                                                                                            prettier/prettier
  342:13  error  Delete `··`                                                                                                                                                                                            prettier/prettier
  347:1   error  Delete `··`                                                                                                                                                                                            prettier/prettier
  355:1   error  Delete `··`                                                                                                                                                                                            prettier/prettier

it's in amqplib tests

@nerumo
Copy link
Contributor Author

nerumo commented Jan 18, 2023

ah, I see. I fixed it, thank you for the other changes.

@Flarna
Copy link
Member

Flarna commented Jan 18, 2023

CI fails for node 18. Not sure why nor if it is caused by this PR as CI logs don't show any useful hint.

@blumamir blumamir merged commit ad92673 into open-telemetry:main Feb 7, 2023
@dyladan dyladan mentioned this pull request Feb 7, 2023
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.

amqplib looses Baggage
3 participants