feat(server-utils): Migrate @opentelemetry/instrumentation-amqplib to orchestrion#21981
feat(server-utils): Migrate @opentelemetry/instrumentation-amqplib to orchestrion#21981s1gr1d wants to merge 2 commits into
Conversation
s1gr1d
requested review from
a team,
JPeer264,
andreiborza,
isaacs,
mydea and
nicohrubec
and removed request for
a team
| const ATTR_MESSAGING_CONVERSATION_ID = 'messaging.conversation_id'; | ||
| const MESSAGING_DESTINATION_KIND_VALUE_TOPIC = 'topic'; | ||
| const MESSAGING_OPERATION_VALUE_PROCESS = 'process'; | ||
| // Inlined (rather than imported) because the `@sentry/conventions` exports are deprecated; the SDK |
There was a problem hiding this comment.
l: I think it is ok to import from @sentry/conventions/attributes here and oxlint-disable this, this is what has been done in other integrations as well AFAIK
| const PUBLISHER_ORIGIN = 'auto.amqplib.orchestrion.publisher'; | ||
| const CONSUMER_ORIGIN = 'auto.amqplib.orchestrion.consumer'; | ||
|
|
||
| // Legacy messaging semantic-conventions, inlined to keep this integration free of `@opentelemetry/*` |
There was a problem hiding this comment.
l: Should we add a v11 todo to check on these attributes? Not sure if there is already a general initiative to change these in v11
| } | ||
| } | ||
|
|
||
| function checkConsumeTimeoutOnChannel(channel: ChannelLike): void { |
There was a problem hiding this comment.
q: Do you think it makes sense to reuse certain functions like this one in the vendored OTel library?
There was a problem hiding this comment.
There's going to be a bunch of these (similar note came up on the nestjs instrumentation). I think the thing to do is sort of just accept that there's a bit of duplication, and then do a sweeping DRY-up refactor.
The reason is we may decide on a few different possible directions once we see it all, so it's kind of high risk of wasted effort to pick one now. Eg:
- delete the OTelJS iitm/ritm based integrations entirely (ie, de-experimental-ize the nodejs orchestrion).
- organize the server-utils/src/integrations/tracing-channel into some set of isolated or bundled library
- improve tree-shaking so only the ones in use get pulled in
- maybe do some kind of dependency-injection at build/preload time for the config and channel definitions.
It's hard to say at this point what's going to be worth the effort, what'll be overkill, etc., so maybe best to just procrastinate the cleanup a little bit.
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Deno test failure is straightforward, just need to add the lib to the default set in the snapshots, looks like.
Once that's resolved, LGTM!
| // The span origin depends on which instrumentation is active. These blocks drive the SDK's default | ||
| // integrations, so when the generic orchestrion run is enabled (via INJECT_ORCHESTRION) the OTel | ||
| // `Amqplib` integration is swapped for the diagnostics-channel one, changing the origin. | ||
| const PUBLISHER_ORIGIN = isOrchestrionEnabled() ? 'auto.amqplib.orchestrion.publisher' : 'auto.amqplib.otel.publisher'; |
| channel.on('close', () => { | ||
| endAllSpansOnChannel(channel, true, END_OP.ChannelClosed, undefined); | ||
| const activeTimer = channel[CHANNEL_CONSUME_TIMEOUT_TIMER]; | ||
| if (activeTimer) { | ||
| clearInterval(activeTimer); | ||
| } | ||
| channel[CHANNEL_CONSUME_TIMEOUT_TIMER] = undefined; | ||
| }); | ||
| channel.on('error', () => { | ||
| endAllSpansOnChannel(channel, true, END_OP.ChannelError, undefined); | ||
| }); |
There was a problem hiding this comment.
If we get an error without a close, could this leave the active timer dangling? If not, ignore this, but it could be good to clear it in either the error or close cases?
| channel.on('close', () => { | |
| endAllSpansOnChannel(channel, true, END_OP.ChannelClosed, undefined); | |
| const activeTimer = channel[CHANNEL_CONSUME_TIMEOUT_TIMER]; | |
| if (activeTimer) { | |
| clearInterval(activeTimer); | |
| } | |
| channel[CHANNEL_CONSUME_TIMEOUT_TIMER] = undefined; | |
| }); | |
| channel.on('error', () => { | |
| endAllSpansOnChannel(channel, true, END_OP.ChannelError, undefined); | |
| }); | |
| // could also move this out of this closure, take the channel as an arg, etc. | |
| function clearActiveTimer() { | |
| const activeTimer = channel[CHANNEL_CONSUME_TIMEOUT_TIMER]; | |
| if (activeTimer) { | |
| clearInterval(activeTimer); | |
| channel[CHANNEL_CONSUME_TIMEOUT_TIMER] = undefined; | |
| } | |
| } | |
| channel.on('close', () => { | |
| endAllSpansOnChannel(channel, true, END_OP.ChannelClosed, undefined); | |
| clearActiveTimer(); | |
| }); | |
| channel.on('error', () => { | |
| endAllSpansOnChannel(channel, true, END_OP.ChannelError, undefined); | |
| clearActiveTimer(); | |
| }); |
| } | ||
| } | ||
|
|
||
| function checkConsumeTimeoutOnChannel(channel: ChannelLike): void { |
There was a problem hiding this comment.
There's going to be a bunch of these (similar note came up on the nestjs instrumentation). I think the thing to do is sort of just accept that there's a bit of duplication, and then do a sweeping DRY-up refactor.
The reason is we may decide on a few different possible directions once we see it all, so it's kind of high risk of wasted effort to pick one now. Eg:
- delete the OTelJS iitm/ritm based integrations entirely (ie, de-experimental-ize the nodejs orchestrion).
- organize the server-utils/src/integrations/tracing-channel into some set of isolated or bundled library
- improve tree-shaking so only the ones in use get pulled in
- maybe do some kind of dependency-injection at build/preload time for the config and channel definitions.
It's hard to say at this point what's going to be worth the effort, what'll be overkill, etc., so maybe best to just procrastinate the cleanup a little bit.
3 participants
Adds
amqplibChannelIntegrationin@sentry/server-utilsfor injecting orchestrion channels intoamqplib.bindTracingChannelToSpan-based subscriber builds publisher/consumer spans; span-building helpers are ported with attributes from@sentry/conventions/attributes, preserving span semantics (long-lived consumer spans, header trace propagation, confirm-channel guard).experimentalUseDiagnosticsChannelInjection()), Bun (automatic), and Deno (denoAmqplibIntegrationwrapper).Closes #20747
Linear: https://linear.app/getsentry/issue/JS-2398/rewrite-opentelemetryinstrumentation-amqplib-to-orchestrion