feat(server-utils): Migrate @opentelemetry/instrumentation-amqplib to orchestrion#21981
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
There was a problem hiding this comment.
I changed all of this again to use the already existing attributes alongside the ones we already use in semantic conventions. Also added two missing attributes: getsentry/sentry-conventions#469
| } | ||
| } | ||
|
|
||
| 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.
There was a problem hiding this comment.
yes this is definitely something we should do in a follow-up
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(); | |
| }); |
There was a problem hiding this comment.
In practice, close should be emitted after error (https://amqp-node.github.io/amqplib/channel_api.html#model_events). But this is still a good hardening of the code 👍
| } | ||
| } | ||
|
|
||
| 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.
# Conflicts: # packages/server-utils/src/orchestrion/channels.ts # packages/server-utils/src/orchestrion/config/index.ts # packages/server-utils/src/orchestrion/index.ts
size-limit report 📦
|
andreiborza
left a comment
andreiborza
left a comment
There was a problem hiding this comment.
Could you please remove the unit tests? They set up an ACS that has no place in unit tests. If functionality like that has to be tested then it should be done in an integration tests.
Most of the stuff in the unit tests should already be covered anyway, but I suggest to pick the differences and move them up.
See #22103 for example.
# Conflicts: # .size-limit.js # packages/server-utils/src/orchestrion/config/index.ts
s1gr1d
force-pushed
the
sig/amqplib-orchestrion
branch
from
3afe4b8 to
5e93d20
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
4 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