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
from
fd71786 to
79b5d89
Compare
RulaKhaled
force-pushed
the
disable-integrations
branch
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
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
| DISABLED_INTEGRATIONS.forEach(integrationName => { | ||
| const index = installedIntegrations.indexOf(integrationName); | ||
| if (index !== -1) { | ||
| installedIntegrations.splice(index, 1); | ||
| } | ||
| }); |
There was a problem hiding this comment.
What do you think of using .filter() here so it's easier to comprehend what the code is doing.
installedIntegrations = installedIntegrations.filter(
integration => !DISABLED_INTEGRATIONS.has(integration)
);| /** | ||
| * Check if an integration has been disabled. | ||
| * @param integrationName The name of the integration to check | ||
| * @returns true if the integration is disabled | ||
| */ | ||
| export function isIntegrationDisabled(integrationName: string): boolean { | ||
| return DISABLED_INTEGRATIONS.has(integrationName); | ||
| } | ||
|
|
||
| /** | ||
| * Remove one or more integrations from the disabled list. | ||
| * @param integrationName The name(s) of the integration(s) to enable | ||
| */ | ||
| export function enableIntegration(integrationName: string | string[]): void { | ||
| if (Array.isArray(integrationName)) { | ||
| integrationName.forEach(name => DISABLED_INTEGRATIONS.delete(name)); | ||
| } else { | ||
| DISABLED_INTEGRATIONS.delete(integrationName); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need those functions in the future?
Right now, they are not used and after calling init the Set will be empty and not used anymore if I understand correctly.
Exposing those methods to users might give them the false impression they can enable/disable integrations even though the method is only checking the Set.
| * Mark one or more integrations as disabled to prevent their instrumentation from being set up. | ||
| * @param integrationName The name(s) of the integration(s) to disable | ||
| */ | ||
| export function disableIntegrations(integrationName: string | string[]): void { |
There was a problem hiding this comment.
I think the UX of the current approach could be improved. For example, the function name implies this is going to disable the integrations, but they are just marked and added to a Set, so no modification is happening at this point. Maybe the naming could be improved a bit so the flow of the functionality is clearer 🤔
Something like this:
- Call
markAsDisabledIntegrationsin the integration setup - Call
removeIntegrationsToDisableininit
Maybe you or @andreiborza have some better ideas for the naming
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().