feat(server-utils): Rewrite @opentelemetry/instrumentation-dataloader to orchestrion#22236
Conversation
size-limit report 📦
|
logaretm
force-pushed
the
feat/dataloader-orchestrion
branch
from
ace18fe to
70ef225
Compare
logaretm
requested review from
JPeer264,
isaacs and
mydea
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
logaretm
force-pushed
the
feat/dataloader-orchestrion
branch
2 times, most recently
from
7ae5c62 to
1578d2e
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
logaretm
force-pushed
the
feat/dataloader-orchestrion
branch
from
1578d2e to
75da927
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some comments. Couldn't really find much to take issue with. It'd be good to add a Deno integration (since broad platform coverage is also the goal here, and it's usually easy to do; if it's not easy, then probably skip it, do it later, etc.)
Also, it'd be nice if there was a test to ensure we get the right nesting and durations, since that was the mistake the clankers found, but tbh, I think it's very unlikely we're going to randomly change a kind: 'Async' to kind: 'Sync' without a very good reason.
| // reliable by `setupOnce`. When the diagnostics channels are injected (runtime hook or bundler | ||
| // plugin), subscribe to them; otherwise fall back to the vendored OTel instrumentation. | ||
| if (isOrchestrionInjected()) { | ||
| dataloaderChannelIntegration().setupOnce?.(); |
There was a problem hiding this comment.
low/consistency nit: It seems like this defines a whole integration, only to call setupOnce on it. Could it just export a subscribeToDataLoaderChannels rather than a full defineIntegration, like how nest does subscribeToNestChannels? (What's here works and is fine, but it's a deviation from the other pattern, and we probably will benefit by either picking one, or deciding when to use one or the other.)
There was a problem hiding this comment.
This is a bit awkward, I think exposing subscribe functions can get muddy like we had with redis.
The .setupOnce?.() reach-in in the node factory is a side effect of the OTel-vs-channel swap. Once we drop the vendored OTel path, there would be no swap and this becomes a normal opt-in integration you add to integrations directly.
Nest is kinda self contained over there, so maybe it's a bit different?
I'd lean toward leaving it, but happy to change it to match if you feel strongly. WDYT?
logaretm
force-pushed
the
feat/dataloader-orchestrion
branch
from
75da927 to
5ee4391
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5ee4391. Configure here.
…r` to orchestrion Replaces the `InstrumentationBase`-based OTel dataloader instrumentation with a diagnostics-channel listener, with orchestrion injecting the channels into `dataloader`'s constructor and prototype methods. `dataloaderIntegration` is opt-in (never a default), so the default-swap in `_init` doesn't apply (a user's explicit instance wins integration dedup). Like `@sentry/nestjs`'s `Nest`, the `@sentry/node` factory picks the channel-vs-OTel path itself at `setupOnce` via `isOrchestrionInjected()`.
logaretm
force-pushed
the
feat/dataloader-orchestrion
branch
from
32e4179 to
bbae2cf
Compare
2 participants
Rewrites the
dataloaderintegration to a diagnostics-channel listener instead ofInstrumentationBase, with orchestrion injecting the channels intodataloader's constructor and prototype methods. Follows the mysql/amqplib pattern, and produces the same spans (load/loadMany/prime/clear/clearAll plus the linked batch span).There was a complication because dataloader is not a default integration, so it requires a different way to wire it in. I followed the same approach as in Nest PR.
The existing node integration test now runs against the real orchestrion path under
INJECT_ORCHESTRION, with the span origin parameterized like amqplib.closes #20749