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: amqplib instrumentation #892

Merged
merged 45 commits into from
Mar 14, 2022
Merged

Conversation

blumamir
Copy link
Member

This is an instrumentation library for the amqplib package which is used for RabbitMQ.

It exists for a year and is currently hosted in ext-js repo, been used and working in production environments, and has 6.4K weekly downloads from npm.

The code is ported as-is with few changes to the hooks signatures.

@blumamir blumamir requested a review from a team February 13, 2022 12:02
@codecov
Copy link

codecov bot commented Feb 13, 2022

Codecov Report

Merging #892 (b6f85e0) into main (99b138f) will decrease coverage by 1.03%.
The diff coverage is 91.74%.

@@            Coverage Diff             @@
##             main     #892      +/-   ##
==========================================
- Coverage   95.91%   94.87%   -1.04%     
==========================================
  Files          13       17       +4     
  Lines         856     1230     +374     
  Branches      178      267      +89     
==========================================
+ Hits          821     1167     +346     
- Misses         35       63      +28     
Impacted Files Coverage Δ
...lugins/node/instrumentation-amqplib/src/amqplib.ts 90.40% <90.40%> (ø)
plugins/node/instrumentation-amqplib/src/utils.ts 95.38% <95.38%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 97.91% <100.00%> (ø)
plugins/node/instrumentation-amqplib/src/types.ts 100.00% <100.00%> (ø)

@blumamir
Copy link
Member Author

Thanks @Flarna , that is a great review. Learned a few new things, very appreciated.
I commented on everything. please resolve the ones that are done

@blumamir
Copy link
Member Author

@Flarna Thanks again for the review. If I am not mistaken, everything is resolved

@Flarna
Copy link
Member

Flarna commented Feb 14, 2022

@blumamir I got interrupted a few times during review. Left two more comments, nothing blocking.

I have to admit that I didn't follow the amqplib patching details as I'm not familiar with this library.

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

overall lgtm (i'm no amplib expert though), i think you'll need to update the release please config as daniel did there though #921

@blumamir
Copy link
Member Author

overall lgtm (i'm no amplib expert though), i think you'll need to update the release please config as daniel did there though #921

done 👍

@blumamir
Copy link
Member Author

I will keep this PR open for another week, giving anyone who wishes to review the chance to do it.
If someone is planning to review but needs more time, please comment here.

@blumamir blumamir merged commit f6c16b6 into open-telemetry:main Mar 14, 2022
@dyladan dyladan mentioned this pull request Mar 14, 2022
@blumamir
Copy link
Member Author

blumamir commented Mar 16, 2022

@blumamir
Copy link
Member Author

PR to fix the daily tests in CI: #946

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.

4 participants