Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Attributes
Names
Other
Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧Deps
Other
🤖 This preview updates automatically when you update the PR. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b2de364 to
fc9eb9d
Compare
|
Little update. After discussing with @Lms24 we decided to prefix everything with |
There was a problem hiding this comment.
I made a deep pass over the attributes and metrics and unfortunately still found a bunch of inconsistencies. Fully aware that they come from bullmq but I think we should adjust them so that our telemetry model is more consistent.
Sorry for not flagging these earlier! If it helps or there are still open things to discuss, feel free to split up the attributes and metrics part so that these can get merged faster.
Maybe in the future I'll split these up to keep the PRs smaller and review more efficient, but from my POV I need both anyways, so it's ok to have it in one.
You brought up amazing points. I addressed all of them, I also checked for other inconsistencies. I just wonder if it would make sense to have these findings somehow in a BUGBOT.md or anything to catch these in the future. |
Lms24
left a comment
There was a problem hiding this comment.
One last note about requirement level of metrics, but once that's sorted, feel free to merge!
| "instrument": "counter", | ||
| "unit": "none", | ||
| "is_in_otel": false, | ||
| "requirement_level": "opt_in", |
There was a problem hiding this comment.
l: Unfortunately, the requirement_level property here is a bit confusing because we never send metrics by default. Users have to opt into sending auto instrumented metrics, so the requirement level here should be interpreted from the perspective of users having opted in already.
I tried to explain this better here and asked Giannis to clarify this for the metric specs.
So I think this metric, as well as most others in this PR that have no potential of becoming super noisy should rather get level "recommended" instead.
| "requirement_level": "opt_in", | |
| "requirement_level": "recommended", |
There was a problem hiding this comment.
I applied that to all metrics
Description
This registers the attributes and metrics that the BullMQ integration of the JavaScript SDK sends (getsentry/sentry-javascript#21777). It also adds the OTel attribute
messaging.client.id(the BullMQ Worker ID) and the OTel metricmessaging.process.duration(in seconds). The metrics arerecommended. The SDK sends them only withenableMetrics: true, and then it sends all of them.Naming decisions
The keys are the BullMQ names with a
messaging.prefix, likemessaging.kafka.*, with snake_case where BullMQ separates words with dots (messaging.bullmq.worker.do_not_wait_active). The metrics get the same prefix.bullmq.job.status(BullMQ before 5.81.2) is sent asmessaging.bullmq.job.state, its BullMQ 6 name.The SDK does not send these, so they are not registered: the attributes it maps to
messaging.*, theJSON.stringify()attributes (bullmq.job.options,bullmq.queue.options,bullmq.worker.options,bullmq.job.progress), andbullmq.job.finished.timestamp, which BullMQ deprecated forbullmq.job.attempt_finished_timestamp.Scrubbing decisions
apply_scrubbingisautofor values from user code (messaging.bullmq.job.deduplication.key,messaging.bullmq.job.key) andmanualfor all others.PR Checklist
yarn testand verified that the tests pass.yarn generateto generate and format code and docs.If an attribute was added:
nextjs.function_id, notfunction_id)apply_scrubbing(i.e.manualorauto. Useneveronly for values that should never be scrubbed such as IDs)If an attribute was deprecated: