Repository navigation
Conversation
1247441 to
212f9d6
Compare
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
|
@JPeer264 can we close this? Is this still relevant? |
|
Actually I still want to ship this, it just had lower prio because of v11. I'll make this pretty next week |
212f9d6 to
589f150
Compare
size-limit report 📦
|
254e5ec to
9d58e2b
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const op = getOpFromSpanName(name); | ||
| const origin = getOriginFromSpanName(name); | ||
|
|
||
| const attributes: SpanAttributes = { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op, | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: origin, | ||
| 'messaging.system': MESSAGING_SYSTEM, | ||
| }; | ||
|
|
||
| if (options?.attributes) { |
There was a problem hiding this comment.
Bug: The BullMQ integration is missing the isRemote: true attribute when creating span links to producer spans, which is inconsistent with the OpenTelemetry specification.
Severity: MEDIUM
Suggested Fix
Add the isRemote: true property to the producerSpanCtx object created in packages/node/src/integrations/tracing/bullmq/tracer.ts. This will align the implementation with the OpenTelemetry specification and ensure correct trace linking for remote spans.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/bullmq/tracer.ts#L58-L67
Potential issue: When creating a span link from a consumer span to a producer span in
the BullMQ integration, the `producerSpanCtx` object is created without the `isRemote:
true` attribute. According to the OpenTelemetry specification, `isRemote` must be set to
`true` when a `SpanContext` represents a remote parent. This omission can cause
incorrect trace visualization and analysis in systems that rely on this field to
identify cross-process communication, as the link will not be correctly identified as
remote. This is inconsistent with other integrations like KafkaJS.
| public addEvent(name: string, attributes?: Record<string, AttributeValue>): void { | ||
| this._span.addEvent(name, attributes ? toOtelAttributes(attributes) : undefined); | ||
|
|
||
| if (name === 'job failed') { |
There was a problem hiding this comment.
Q: where does this string come from? Is this always emitted this way by BullMQ? Could this change?
There was a problem hiding this comment.
I added a comment about it. It comes from BullMQ directly: https://github.com/taskforcesh/bullmq/blob/01b8b14a973984845dc0bbcefccf35b3ed30782d/src/classes/worker.ts#L1223-L1225
Don't think it would change as it is in there since the beginning in their repo
| span.addLink({ | ||
| context: producerSpanCtx, | ||
| attributes: { | ||
| [SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace', |
There was a problem hiding this comment.
sentry-conventions now also exports this (you might need to upgrade the version): https://getsentry.github.io/sentry-conventions/attributes/sentry/#sentry-link-type
| // TODO(v11): Remove this once EAP can store span links. We currently only set this attribute so that we | ||
| // can obtain the previous trace information from the EAP store. Long-term, EAP will handle | ||
| // span links and then we should remove this again. | ||
| span.setAttribute( | ||
| 'sentry.previous_trace', | ||
| `${producerSpanCtx.traceId}-${producerSpanCtx.spanId}-${producerSpanCtx.traceFlags}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
EAP can store links, so this is not necessarily needed. We rather need to update the frontend code in Sentry (so the comment is not 100% true). Also, v11 is already out.
Check those files for reference:
| function getOpFromSpanName(name: string): string { | ||
| const operation = getOperation(name); | ||
|
|
||
| if (CONSUMER_OPERATIONS.has(operation)) { | ||
| return 'queue.task'; | ||
| } | ||
|
|
||
| if (PRODUCER_OPERATIONS.has(operation)) { | ||
| return 'queue.submit'; | ||
| } | ||
|
|
||
| return 'queue'; | ||
| } |
There was a problem hiding this comment.
M: We don't have those OPs (queue.task and queue.submit) yet in conventions. See:
There was a problem hiding this comment.
Amazing. I'm pretty sure back in June, when I implemented that, it wasn't there. So great that this got added now.
| public setAttribute(key: string, value: AttributeValue): void { | ||
| this._span.setAttribute(key, toOtelAttributeValue(value)); | ||
| } | ||
|
|
||
| public setAttributes(attributes: Record<string, AttributeValue>): void { | ||
| this._span.setAttributes(toOtelAttributes(attributes)); | ||
| } |
There was a problem hiding this comment.
Will this set the attributes we define in conventions? (if we can get them)
https://getsentry.github.io/sentry-conventions/attributes/messaging/
Can you also make sure to check for the attributes in the tests.
There was a problem hiding this comment.
That's a good one. Actually I'm going to ask how we should proceed on this, as there are tons of attributes defined by BullMQ
|
|
||
| const span = | ||
| op === 'queue.task' | ||
| ? withActiveSpan(null, () => startInactiveSpan({ name, attributes })) |
There was a problem hiding this comment.
M: Can you add a test that processes two jobs and compare their trace_id. I think we would get the same ID for all jobs (it should rather use a separate trace ID for each job in the worker).
There was a problem hiding this comment.
integration test has been added 🤝
| const span = | ||
| op === 'queue.task' | ||
| ? withActiveSpan(null, () => startInactiveSpan({ name, attributes })) | ||
| : startInactiveSpan({ name, attributes }); |
There was a problem hiding this comment.
got this from my agent: internal operations (moveStalledJobsToWait or pause) would start root spans and this could created a lot of them as some operations start on a schedule. Maybe we could add onlyIfParent: true for spans that are neither a producer or consumer. Or this:
Implement the optional
ContextManager.root()that BullMQ added forwithDetachedContext. Without it, a worker created inside a span keeps the stalled checker attached to that span for the whole process lifetime (see BullMQ's comment installedChecker()). Note: todaywith()keeps the ambient span whencontext.spanisundefined, soroot()also needswithActiveSpan(null, fn)in that branch.
There was a problem hiding this comment.
Actually, good call instict. I added the onlyIfParent and made it depending on the OP.
|
|
||
| if (name === 'job failed') { | ||
| const reason = attributes?.['bullmq.job.failed.reason']; | ||
| captureException(new Error(String(reason || 'Unknown error')), { |
There was a problem hiding this comment.
Would we still have access to the callstack here? We could pass that.
There was a problem hiding this comment.
Not at the moment, as BullMQ doesn't expose that. They only expose the messge: https://github.com/taskforcesh/bullmq/blob/01b8b14a973984845dc0bbcefccf35b3ed30782d/src/classes/worker.ts#L1224
Could be a nice upstream addition though. I will follow up on it
|
👋 @chargome, @nicohrubec — Please review this PR when you get a chance! |
|
👋 @chargome, @nicohrubec — Please review this PR when you get a chance! |
6d0304a to
b8e696a
Compare
| * | ||
| * @see https://docs.bullmq.io/guide/telemetry | ||
| */ | ||
| export class BullMQTelemetry implements Telemetry<SentryContext> { |
There was a problem hiding this comment.
m: let's make metric collection opt in, given enableMetrics no longer exists
There was a problem hiding this comment.
I added enableMetrics as a dedicated option in the integration
| span.addLink({ | ||
| context: producerSpanCtx, | ||
| attributes: { | ||
| [SENTRY_LINK_TYPE]: 'previous_trace', |
There was a problem hiding this comment.
m: Should we define a different link type than previous_trace for this link? previous_trace was meant for frontend-subsequent traces. The buttons in the UI mention to "go to the previous trace of the same session", which doesn't really fit the producer/consumer pattern.
WDYT?
There was a problem hiding this comment.
Theoretically it is still the previous trace, as the consumer's previous trace is the producer. But we can definitely change that to consumer_origin or something like that. I assume we're quite free in choosing a name here?!
Or producer_trace to keep it aligned with *_trace
There was a problem hiding this comment.
I assume we're quite free in choosing a name here?!
Yes, feel free to choose whatever, but update conventions and this develop docs page. I think an extra type might make sense because one producer might trigger multiple consumers, which is something previous_trace wasn't designed to handle.
worth syncing with @s1gr1d since she added cache_origin very recently :)
There was a problem hiding this comment.
I think an extra type might make sense because one producer might trigger multiple consumers
Yes they can have more. It can also happen that within one trace there are multiple producers that trigger multiple consumers. But on the consumer side there is only 1 link back to its origin
There was a problem hiding this comment.
What about message_origin? It follows the same pattern as cache_origin. cache_origin points to the span where a cached value came from, and message_origin points to the span where a message came from.
I would avoid consumer_origin because it reads like "where the consumer came from". The thing with an origin is the message (the job), not the worker.
There was a problem hiding this comment.
Btw, the link type is now shown in the "Links" component, so you get a bit more info now: getsentry/sentry#126701
| public with<A extends (...args: unknown[]) => unknown>(context: SentryContext, fn: A): ReturnType<A> { | ||
| if (context.span) { | ||
| return withIsolationScope(() => { | ||
| return withActiveSpan(context.span as Span, fn) as ReturnType<A>; | ||
| }); | ||
| } | ||
| return withIsolationScope(() => withActiveSpan(null, fn)) as ReturnType<A>; | ||
| } |
There was a problem hiding this comment.
Bug: The BullMQ contextManager.with() method calls withIsolationScope(), which overwrites the trace context, leading to mismatched trace IDs for job spans and subsequent events.
Severity: MEDIUM
Suggested Fix
In the contextManager.with() method, before activating the span, restore the propagation context from the span's captured scopes. This can be done by retrieving the creationScope from the span and applying its propagation context to the current scope, mirroring the logic in the runCallback() helper.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/bullmq/contextManager.ts#L20-L27
Potential issue: When processing a BullMQ job, a span is created with a new trace
context. The `contextManager.with()` method then calls `withIsolationScope()`, which
incorrectly generates a new, different trace context, overwriting the original one.
Subsequently, `withActiveSpan()` sets the original span as active, but the scope it
operates in now has a mismatched propagation context. As a result, any events captured
during the job, such as exceptions via `captureException()`, will be associated with the
wrong trace ID, breaking distributed tracing for BullMQ jobs.
| if (name === 'job failed') { | ||
| const reason = attributes?.['bullmq.job.failed.reason']; | ||
| captureException(new Error(String(reason || 'Unknown error')), { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.queue.bullmq', | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: All failed BullMQ jobs are grouped into a single Sentry issue because they share the same synthetic stack trace, making it difficult to distinguish between different failure types.
Severity: MEDIUM
Suggested Fix
Capture the original Error object from the BullMQ failure if it is available. Alternatively, use a Sentry fingerprint override to group issues by the actual error message rather than the synthetic stack trace origin.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/bullmq/span.ts#L77-L85
Potential issue: When a BullMQ job fails, a new synthetic `Error` is created within the
integration. Because all these synthetic errors originate from the same code location,
they share an identical stack trace. Sentry's primary grouping mechanism relies on the
stack trace, which will cause it to group all distinct BullMQ job failures (e.g., 'Redis
connection refused' and 'Timeout waiting for response') into a single issue. This
prevents users from effectively distinguishing between and triaging different types of
job failures in Sentry.
| // https://github.com/taskforcesh/bullmq/blob/01b8b14a973984845dc0bbcefccf35b3ed30782d/src/classes/worker.ts#L1223-L1225 | ||
| if (name === 'job failed') { | ||
| const reason = attributes?.['bullmq.job.failed.reason']; | ||
| captureException(new Error(String(reason || 'Unknown error')), { |
There was a problem hiding this comment.
Bug: If a BullMQ job fails with an error that has an empty string message, the reason is incorrectly reported as 'Unknown error' due to a faulty falsy check.
Severity: LOW
Suggested Fix
Use the nullish coalescing operator (??) instead of the logical OR operator (||) to handle the fallback. Change reason || 'Unknown error' to reason ?? 'Unknown error' to correctly handle '' as a valid reason while still providing a fallback for null or undefined.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/bullmq/span.ts#L79
Potential issue: In the BullMQ integration, when a job fails, the failure reason is
captured. The code uses the expression `reason || 'Unknown error'` to provide a fallback
message. However, if a job fails with an error that has a valid but empty message
(`''`), the expression `'' || 'Unknown error'` incorrectly evaluates to `'Unknown
error'`. This causes the original context of the failure (an empty message) to be lost
and replaced with a generic, misleading message in the captured Sentry event.
This branch has not been deployed
4 participants
closes #12956
closes JS-89
Description
This adds telemetry for BullMQ. It only piggybacks on the
telemetryoption. It has metrics, tracing with span links between producer and consumerIt can be used as following:
It is the same usage as the BullMQ OTel package: https://docs.bullmq.io/guide/telemetry/getting-started
To make this an integration the types were vendored in and based on that the integration was done.
OP decisions
The consumer spans generate
queue.taskwhere the producer emitsqueue.submit, this should be reflecting our span operations. I didn't usequeue.task.bullmq, like it is for Celery - but this can be changed if wanted.Sentry traces / metrics
Example metrics: https://sentry-sdks.sentry.io/explore/metrics/?end=2026-10-05T07%3A25%3A00&metric=%7B%22metric%22%3A%7B%22name%22%3A%22bullmq.jobs.completed%22%2C%22type%22%3A%22counter%22%2C%22unit%22%3A%22none%22%7D%2C%22query%22%3A%22%22%2C%22aggregateFields%22%3A%5B%7B%22yAxes%22%3A%5B%22sum%28value%2Cbullmq.jobs.completed%2Ccounter%2Cnone%29%22%5D%7D%5D%2C%22aggregateSortBys%22%3A%5B%7B%22field%22%3A%22sum%28value%2Cbullmq.jobs.completed%2Ccounter%2Cnone%29%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22sortBys%22%3A%5B%7B%22field%22%3A%22timestamp%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22mode%22%3A%22samples%22%7D&project=4510555608449024&start=2026-10-05T07%3A15%3A00
Consumer trace (queue.task span with a link to the producer): https://sentry-sdks.sentry.io/explore/traces/trace/1a21906f0d8d4db7bbf80bf9cf7d32f3/?node=span-9b305b00fd106329&project=4510555608449024&source=traces&targetId=9b305b00fd106329×tamp=1791184722
Producer trace (GET /enqueue with a queue.submit span): https://sentry-sdks.sentry.io/explore/traces/trace/f135329fd15a440091a2ee445a8df4fc/?node=span-9db4d2c0668abe46&project=4510555608449024&source=traces&targetId=9db4d2c0668abe46×tamp=1791184722