feat(node): Ensure prismaIntegration works with Prisma v5
#17595
Conversation
There was a problem hiding this comment.
not really related but noticed we had these checked in for no reason.
| expect(spans).toContainEqual( | ||
| expect.objectContaining({ | ||
| data: { | ||
| 'db.statement': expect.stringContaining('SELECT'), |
There was a problem hiding this comment.
these tests were missing and this was actually what was not working in v5 without passing in the custom instrumentation, previously. this is now fixed on this branch.
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
mydea
changed the title
prismaIntergation works with Prisma v5prismaIntegration works with Prisma v5
| const spanId = engineSpan.span_id; | ||
| const traceId = engineSpan.trace_id; | ||
|
|
||
| const links: Link[] | undefined = engineSpan.links?.map(link => { |
There was a problem hiding this comment.
q: any idea what links prisma instrumentation would add to their spans? Just curious since this one of the first links usages I see in our code, no complaints :D
There was a problem hiding this comment.
no idea, I just copied this from their internal implementation of this where they allow for this :D Maybe a good link to drop here, I tried to make this work based on this:
| }, | ||
| }; | ||
|
|
||
| tracer._idGenerator = temporaryIdGenerator; |
There was a problem hiding this comment.
I'm wondering if this works reliably. Probably not because users can configure tools like terser to minify private fields (like we do in our CDN bundles). But then again, quite unlikely to happen in server builds. So let's roll with it :D
There was a problem hiding this comment.
yeah I had the same thought. this is def. not bullet proof 😬 for now I added a log about this, if this does not work then you will miss out on some spans (the db spans themselves) which is not great but I guess it is OK for this hacky compatibility work for the old version!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } finally { | ||
| // Ensure we always restore this at the end, even if something errors | ||
| tracer._idGenerator = initialIdGenerator; | ||
| } |
There was a problem hiding this comment.
Bug: Prisma Span Missing Attributes, Tracer ID Conflict
Prisma v5 compatibility spans created in createEngineSpan are missing mandatory Sentry attributes like SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN and SEMANTIC_ATTRIBUTE_SENTRY_OP. Additionally, the temporary global override of tracer._idGenerator (lines 127-135) creates a race condition, potentially causing other concurrently created spans to receive incorrect IDs.
There was a problem hiding this comment.
this is correct in theory but we add the attributes etc. in a separate place.
Once getsentry/sentry-javascript#17595 is merged support will be even better, but even today passing the `prismaInstrumentation` will actually fail, so not doing it is better even if some spans are missing. Also updating the docs to reflect that this is enabled by default now.
This PR fixes the usage of the SDK in v10 with Prisma v5.
We used to require/recommend that users manually installed
@prisma/instrumentationv5 and passed this to ourprismaIntegrationlike this:However, this no longer works in v10 because that version pulls in v1 of
@opentelemetry/sdk-trace-base, which is no longer compatible in v10 of our SDK 😢So to fix this, this PR removes/deprecates the passing of
prismaInstrumentation(this now no-ops). v5 is now supported out of the box!To make this work, some hacks and workarounds were necessary 😬 The problem is that v5 of the prisma instrumentation relies on manually creating span instances with specified ID and parent IDs. This is no longer possible in OTEL v2, as all these APIs are no longer exported 😞 (it was also weird from prisma to do this in the first place, and they no longer do this in v6 of the instrumentation).
To get this to work, we manually overwrite the
tracer._idGeneratorwith a mock that returns the IDs we need, kind of making this work as expected again. Hopefully this will not break in the future, we'll see - our tests should at least show us if that ever breaks.While at this, I also re-wrote our integration tests to run in CJS and ESM properly and use the new test runner structure instead of manually calling yarn etc. in there.
Example trace before in v5 (without custom prismaInstrumentation, which broke at runtime):
https://sentry-sdks.sentry.io/explore/discover/trace/49e2095162e4bb571074e27653586643/?dataset=transactions&field=title&field=project&field=user.display&field=timestamp&name=All%20Errors&node=span-61995a9d80c6925f&project=4508330866769920&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=1h×tamp=1757575919&yAxis=count%28%29
Example trace with this PR:
https://sentry-sdks.sentry.io/explore/discover/trace/deaaa1990ba6204085779aa09888dc2c/?dataset=transactions&field=title&field=project&field=user.display&field=timestamp&fov=0%2C813.7861328125&name=All%20Errors&node=span-8eb41978be76824e&project=4508330866769920&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=1h×tamp=1757575780&yAxis=count%28%29