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(bullmq): add bullmq (>=2) instrumentation #2293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

unflxw
Copy link
Contributor

@unflxw unflxw commented Jun 21, 2024

Add an instrumentation for BullMQ. This commit adds the already existing instrumentation at appsignal/opentelemetry-instrumentation-bullmq to this repository, with some minor changes, removing under-tested functionality in order to reduce the maintenance surface.

The initial work for this instrumentation was done by @jenniferplusplus at jenniferplusplus/opentelemetry-instrumentation-bullmq, of which both appsignal/opentelemetry-instrumentation-bullmq and the instrumentation in this commit are forks.

The instrumentation aims to comply with the Messaging Semantic Conventions (v0.24.0) whenever possible. Due to the latest published version of the @opentelemetry/semantic-conventions package not fully complying with the semantic conventions, when in conflict, the instrumentation chooses to follow the implemented shared package instead of the contents of the document.

@unflxw unflxw requested a review from a team June 21, 2024 13:09
@unflxw unflxw force-pushed the bullmq-instrumentation branch 3 times, most recently from 2474614 to 873b146 Compare June 21, 2024 15:11
@jenniferplusplus
Copy link

It's good to see this happening. I'm glad the instrumentation has been useful to people, but I'm also glad to be relieved of it. It's not a stack I use anymore.

I'll be happy to mark my npm package as deprecated once this gets merged and published.

@unflxw
Copy link
Contributor Author

unflxw commented Jun 21, 2024

Thank you!

The same goes for @appsignal/opentelemetry-instrumentation-bullmq, we would deprecate it and switch our dependencies over to the contrib instrumentation.

@pichlermarc
Copy link
Member

Hi @unflxw. We've recently added new contributing guidelines for new instrumentation libraries.

Is there a specific reason why you'd want @appsignal/opentelemetry-instrumentation-bullmq here in this repo as opposed to hosting and maintaining it in a seperate repo, ideally closer to the community that knows and maintains BullMQ?

@unflxw
Copy link
Contributor Author

unflxw commented Jun 26, 2024

@pichlermarc Thanks for letting me know! I had not seen these guidelines before.

Our belief is that it is valuable for this instrumentation to be visible to the OpenTelemetry community, as well as for its maintenance to be shared with the community at large. Nonetheless, I have added myself as the maintainer for this instrumentation in the component owners file.

We're not BullMQ maintainers ourselves. We run an OpenTelemetry-based APM, and we took on the task of improving this instrumentation in order to be able to offer it to our customers. So, while we can host it on a public repository of our own, as we currently do, it will not be a repository owned by the BullMQ maintainers. The BullMQ maintainer team has stated that they're not interested in maintaining the OpenTelemetry instrumentation.

The instrumentation, while popular enough to have tens of thousands of weekly downloads, was not under active maintenance. Part of why we forked it was to avoid imposing additional maintenance workload on @jenniferplusplus, as we implemented changes to it. We would hope to prevent further fragmentation of the instrumentation ecosystem and the work on it by sponsoring it under the OpenTelemetry banner.

Add an instrumentation for BullMQ. This commit adds the already existing
instrumentation at appsignal/opentelemetry-instrumentation-bullmq, with
some minor changes to reduce the functionality and maintenance surface.

The initial work for this instrumentation was done by @jenniferplusplus
at jenniferplusplus/opentelemetry-instrumentation-bullmq, of which both
appsignal/opentelemetry-instrumentation-bullmq and the instrumentation
in this commit are forks.

The instrumentation aims to comply with the Messaging Semantic Conventions
(v0.24.0) whenever possible. Due to the latest published version of the
`@opentelemetry/semantic-conventions` package not fully complying with
the semantic conventions, when in conflict, the instrumentation chooses
to follow the implemented shared package instead of the contents of the
document.
@unflxw
Copy link
Contributor Author

unflxw commented Jun 26, 2024

I have added @luismiramirez as a code owner for the instrumentation, in compliance with the guidelines.

Copy link

codecov bot commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 98.59813% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.54%. Comparing base (dfb2dff) to head (cb5868e).
Report is 173 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2293      +/-   ##
==========================================
- Coverage   90.97%   90.54%   -0.44%     
==========================================
  Files         146      150       +4     
  Lines        7492     7477      -15     
  Branches     1502     1556      +54     
==========================================
- Hits         6816     6770      -46     
- Misses        676      707      +31     
Files Coverage Δ
...gins/node/instrumentation-bullmq/src/attributes.ts 100.00% <100.00%> (ø)
...ugins/node/instrumentation-bullmq/src/.eslintrc.js 0.00% <0.00%> (ø)
...node/instrumentation-bullmq/src/instrumentation.ts 99.03% <99.03%> (ø)

... and 59 files with indirect coverage changes

@unflxw
Copy link
Contributor Author

unflxw commented Jul 29, 2024

Please let us know if there's anything we can do to help move this forward.

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