feat(node-core): Add integration disabling mechanism to prevent instrumentation conflicts #17972
Conversation
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.
|
RulaKhaled
force-pushed
the
disable-integrations
branch
2 times, most recently
from
470034b to
31cce57
Compare
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
Bug: Race Condition in AI Provider SetupA race condition exists where the LangChain integration's |
RulaKhaled
changed the title
RulaKhaled
changed the title
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
Nice! I added some comments regarding the current naming of the functions as they kinda imply a different meaning to what they are actually doing and I think it would be good to give this another look when we expose those functions to our users.
And maybe it's also worth to add some tests
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/node/src/integrations/tracing/langchain/instrumentation.ts
Outdated
Show resolved
Hide resolved
|
@s1gr1d Aside from resolving the comments mentioned above, I made a quick refactor to the PR: Previously, we unregistered all disabled integrations even if their modules weren’t required at runtime (i.e., when the patch wasn’t fired), which broke AI instrumentations. We also couldn’t have handled marking an integration as disabled in the patch while handling disabling in the setup logic, because that would’ve been too late, the integration would’ve already been registered during setup. To fix this, I moved the registration of disabled integrations into the patch, ensuring it only happens when the module is actually used. Disabled integrations are now unregistered in _patch(), which serves as a good middle ground for now since we don’t want to require customers to disable integrations manually. Ideally, we’ll support disabling integrations earlier (at setup time), possibly via lazy loading? but that’s TBD and out of scope for this PR. |
andreiborza
left a comment
andreiborza
left a comment
There was a problem hiding this comment.
The approach we landed on seems okay to me, but I think we should be more specific with the naming of it.
Initially, we planned to disable integrations but after a few iterations we landed on skipping wrapping certain ai providers at runtime. I think we can reflect that change in the naming and really just scope this feature to ai providers.
I'd call the methods:
_skipAiProviderWrapping(providers: string[])
_shouldSkipAiProviderWrapping(provider: string)
_clearAiProviderSkips()
| // (e.g., when LangChain skips OpenAI in one client, but a subsequent client uses OpenAI standalone) | ||
| _INTERNAL_clearAiProviderSkips(); | ||
| super._setupIntegrations(); | ||
| } |
There was a problem hiding this comment.
Bug: Reinit Breaks Instrumentation Skip Logic
Calling _INTERNAL_clearAiProviderSkips() in _setupIntegrations() can cause incorrect instrumentation when Sentry.init() is called multiple times. If LangChain was imported during the first initialization and marked AI providers to skip, a second initialization clears those markers. Since LangChain's _patch won't run again (module already loaded), new AI provider client instances created after the second init will be incorrectly instrumented, even though LangChain is active. The skip markers should persist across client reinitializations or only be cleared when LangChain's patch logic runs.
There was a problem hiding this comment.
This is a limitation of the approach taken, should probably be fine in most cases.
andreiborza
force-pushed
the
disable-integrations
branch
from
d00cd78 to
baebb0d
Compare
When using higher-level integrations that wrap underlying libraries, both the wrapper integration and the underlying library integration can instrument the same API calls, resulting in duplicate spans. This is particularly problematic for:
The disabled integrations mechanism can be used as follows:
and is used in LangChain to auto disable OpenAI, Anthropic AI, and Google GenAI integrations in
setupOnce().