feat(node): Only add span listeners for instrumentation when used #15802
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mydea
force-pushed
the
fn/conditional-spanStart
branch
from
90d77ae to
ee30339
Compare
size-limit report 📦
|
mydea
changed the title
mydea
commented
Mar 25, 2025
| @@ -119,6 +125,12 @@ export const prismaIntegration = defineIntegration( | |||
| instrumentPrisma({ prismaInstrumentation }); | |||
| }, | |||
| setup(client) { | |||
| // If no tracing helper exists, we skip any work here | |||
There was a problem hiding this comment.
this is not perfect as possibly prisma could be set up later than setup(client) is being called, but it was the best I could come up with here 🤔 it seems to work in tests at least!
AbhiPrasad
approved these changes
Mar 25, 2025
| } | ||
|
|
||
| return event; | ||
| instrumentation = instrumentVercelAi(); |
There was a problem hiding this comment.
We should convert this to use instrumentWhenWrapped.
There was a problem hiding this comment.
I was thinking about this - technically, this implementation is better and safer, as it does not rely on patching the method etc. Since we control the instrumentation class here 🤔
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #15725 (comment), this PR is an idea to avoid registering
on('spanStart')hooks for all the node libraries that do not have proper hooks, unless they are used.Today, for OTEL instrumentation that does not provide span hooks, we fall back to
client.on('spanStart')to enhance/mutate spans emitted by the instrumentation. This PR improved this by only registering thespanStartclient hook if we detect that the OTEL instrumentation is actually wrapping something. This avoids us calling a bunch of span callbacks each time a span is started, when it is not even needed.For this, a new
instrumentWhenWrappedutility is added which can be passed an instrumentation. This works by monkey-patching_wrapon the instrumentation, and ensuring a callback is only called conditionally.Note: For some (e.g. connect, fastify) we register
spanStartin the error handler, which is even better, so there we do not need this logic.