feat(core): Add extendIntegration method#21759
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 9767b7b. Configure here.
|
|
||
| const wrappedFunction = function (this: unknown, ...args: unknown[]): unknown { | ||
| baseBound(...args); | ||
| return extendedBound(...args); |
There was a problem hiding this comment.
processEvent chain drops base result
Medium Severity
When extendIntegration wraps overlapping processEvent hooks, it invokes the parent with the original arguments and ignores its return value, then runs the extension with the same original event. Parent mutations, null drops, and async results are not forwarded, so extended integrations can observe or send events the parent meant to filter or replace.
Reviewed by Cursor Bugbot for commit 9767b7b. Configure here.
There was a problem hiding this comment.
yeah, this is kind of "expected" here. it will not really work for this usecase 🤔 not sure if we should tighten this so it only actually covers certain things (e.g. setup, setupOnce) or keep it generic like it is now 🤔
Basically it does not work for anything that expects a return value from the method.
size-limit report 📦
|
mydea
requested review from
JPeer264,
Lms24,
andreiborza,
chargome,
logaretm,
nicohrubec and
s1gr1d
and removed request for
a team
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const wrappedFunction = function (this: unknown, ...args: unknown[]): unknown { | ||
| baseBound(...args); | ||
| return extendedBound(...args); |
There was a problem hiding this comment.
Bug: The extendIntegration wrapper discards the return value of the base method. For processEvent, this ignores decisions to drop events and creates race conditions with async operations.
Severity: MEDIUM
Suggested Fix
The wrappedFunction inside extendIntegration should handle the return value of the base method. For processEvent, it should await the result if it's a promise and pass the modified event to the extended method. It should also check for null and propagate the decision to drop the event by returning null immediately.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/integration.ts#L220-L222
Potential issue: The `extendIntegration` function wraps methods by calling the base
integration's method and then the extended integration's method. However, the return
value of the base method call (`baseBound(...args);`) is discarded. This is particularly
problematic for the `processEvent` hook, which can return `null` to drop an event or a
`Promise` with a modified event. By ignoring the return value, the wrapper will not drop
events when the base integration intends to, and it will create a race condition if the
base `processEvent` is asynchronous, as the extended method will be called immediately
with the original event.
Did we get this right? 👍 / 👎 to inform future reviews.
| options?: HttpServerSpansIntegrationOptions, | ||
| ) => Integration & { | ||
| name: 'HttpServerSpans'; | ||
| name: 'Http.ServerSpans'; |
There was a problem hiding this comment.
m: ist the name change intentional? (I like the new pattern but just wanted to flag)
There was a problem hiding this comment.
this was actually incorrect, if you look the actual name of the integration is Http.ServerSpans - one more reason why typing it this way is actually harmful. In v11 we should remove this cast probably and just let this be inferred.
| options?: HttpServerIntegrationOptions, | ||
| ) => Integration & { | ||
| name: 'HttpServer'; | ||
| name: 'Http.Server'; |
There was a problem hiding this comment.
m: ist the name change intentional? probably a fix for the INTEGRATION_NAME diversion
There was a problem hiding this comment.
same here, this just fixes the type to the actual name this has 😅
| const extendedBound = extendedValue.bind(wrappedIntegration) as typeof extendedValue; | ||
|
|
||
| const wrappedFunction = function (this: unknown, ...args: unknown[]): unknown { | ||
| baseBound(...args); |
There was a problem hiding this comment.
l: we could warn if we see that the return type of baseBound is not undefined
but yeah, as we discussed offline, it's not required to use this helper should we ever extend an integration with process* hooks, so not a blocker for me
| const wrappedFunction = function (this: unknown, ...args: unknown[]): unknown { | ||
| baseBound(...args); | ||
| return extendedBound(...args); | ||
| } as (Omit<Base, keyof Extended> & Extended)[Extract<keyof Extended, string>]; |
There was a problem hiding this comment.
nit: maybe this can be extracted as a type to make it more readable
3 participants
This adds a new method,
extendIntegration, that can be used to safely extend an integration.Today, we have the problem that if we extend an integration (e.g. node extending something from server-utils), we have to manually call the parent integration functions. This is a bit brittle because a) it requires you to know exactly what methods the parent integration has, which we usually want to abstract away from users on purpose. also, if a parent integration changes, this could be forgotten/lost.
Usage:
vs. before:
In addition, I also improved
defineIntegrationto maintain a static name, and made all integration names static for easier access.Note: We have a bunch of places where we started to type integrations manually, e.g.
We should revert this in v11. We purposefully did not do this because we want to keep the underlying implementation flexible. I think this was done sometimes to make it easier to extend/call things, but usually this should not be necessary and the extend helper should help with making things type safe a bit easier.