feat(bun,deno,node): pg orchestrion instrumentation#21826
Conversation
isaacs
requested review from
JPeer264,
andreiborza and
mydea
and removed request for
a team
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
🚲 🏠 API surface choice: In order to opt into orchestrion diagnostics-channel-injection style instrumentations, which swap out the vendored OTel instrumentations, we call This initializes the orchestrion integrations, and uses them instead of the OTel ones. However, this means that the only time that the orchestrion integrations are set up and provided with options, is at that moment of calling Sentry.experimentalUseDiagnosticsChannelInjection({ postgres: { ignoreConnectSpans: true } });I don't love this! It exposes the implementation detail in a way that we will likely regret. I kind of hate it, actually.
It was a simple duct-tape way forward, but it sucks. Another approach would be, instead of actually initializing these integrations at that moment to swap in for the OTel ones, just mark that we're going to use the orchestrion ones when we do initialize, and then when it comes time to initialize them, choose which implementation to use, with the configurations provided. I'm going to try spiking that out here. EDIT: yeah, fixed it. Much nicer. The |
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
fbbe3ae to
334614a
Compare
size-limit report 📦
|
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
9545bf8 to
dfe74f5
Compare
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Overall LGTM, got minor suggestions. I'll review once more after the tests are green and the merge conflicts got solved
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
dfe74f5 to
373c014
Compare
|
👋 @mydea, @andreiborza — Please review this PR when you get a chance! |
mydea
changed the title
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
373c014 to
f50ea58
Compare
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
2 times, most recently
from
afbbc64 to
62a281f
Compare
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
2 times, most recently
from
668eabf to
b2621bd
Compare
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
b2621bd to
58fdc22
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 58fdc22. Configure here.
Add orchestrion instrumentation for Node, Deno, and Bun, covering the `pg` module. This basically copies exactly what the `mysql` integration does, but for postgres. fix: #20764 fix: JS-2415
Make it possible to provide options to orchestrion integrations without needing to access internal API `@sentry/server-utils/orchestrion`. An integration test is added verifying that `ignoreConnectSpans` can be set on the orchestrion `pg` integration.
Move the list of orchestrion integrations, keyed by their public-facing OTel names, into server-utils. This allows us to easily provide them in a user-visible way from the Node SDK, and also avoid forgetting to add them in multiple places as we add new ones to the set.
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
2b34039 to
a4e321e
Compare
|
@mydea @JPeer264 Updated this to remove the overly-complicated factory-fork approach, so this is now in the "export the server-utils/orchestrion integrations in a more ergonomic way" approach to setting options. Also updated #21828 to do the same thing it was doing, but with this new simpler underlying layer. |
3 participants
Add orchestrion instrumentation for Node, Deno, and Bun, covering the
pgmodule.This basically copies exactly what the
mysqlintegration does, but for postgres.fix: #20764
fix: JS-2415