fix(core): Fix integration deduping #5696
Conversation
size-limit report 📦
|
| * Remove duplicates from the given array, preferring the last instance of any duplicate. Not guaranteed to | ||
| * preseve the order of integrations in the array. |
There was a problem hiding this comment.
| * Remove duplicates from the given array, preferring the last instance of any duplicate. Not guaranteed to | |
| * preseve the order of integrations in the array. | |
| * Remove duplicates from the given array, preferring the last instance of duplicate instances, and user instances over | |
| * default instances. Not guaranteed to preseve the order of integrations in the array. |
There was a problem hiding this comment.
Technically? Yes. But I'd argue that
a) it falls under the heading of "fixing bugs is always technically breaking", and
b) coming back to the examples from the description, we have
[UserProvidedCopyofIntegration, DefaultCopyOfSameIntegration]- the user's copy should win, and already does (no behavior change here)[DefaultCopyofIntegration, UserProvidedCopyOfSameIntegration]- the user's copy should win, but currently doesn't (this seems wrong on its face and also, why would anyone rely on us ignoring their copy?)[DefaultCopyAofIntegration, DefaultCopyBofIntegration]- copy B should win, but currently doesn't (we have total control here, though I don't think it actually comes up anywhere)[UserCopyAofIntegration, UserCopyBofIntegration]- copy B should win, but currently doesn't (the one breaking, not-currently-quite-as-obviously-wrong change, but I'm wagering that the number of people actively providing two copies of the same integration is vanishingly small)
I honestly have no idea why we were preferring first to begin with (the PR which introduced both the logic and the test unfortunately says absolutely nothing about it), and I can't find any place where we rely on that behavior, so I feel okay changing it.
WDYT?
There was a problem hiding this comment.
lol at the xkcd
I am totally fine with the changes. If people report in that they see problems, we can tell them to change the order to match the fixed behavior!
There was a problem hiding this comment.
Whoops - completely forgot to include your good suggestion above before I merged. I'll throw it into the pile for the next comments-fix PR.
When the SDK is initialized, we set up a combination of default and user-provided integrations, controlled by two
Sentry.init()options:defaultIntegrations- can be set tofalseto disable defaultsintegrations- can be an array of integrations (to be merged with the existing array of defaults) or a function(defaults: Integration[]) => Integration[](to replace the entire array of defaults with a new array)The first option works correctly, but the second has long had two bugs, both relating to how duplicates are handled. Specifically, when two instances of the same integration are present, the following two principles should (but don't) always hold: 1) a user-provided instance should always beat a default instance, and within that, 2) a more recent instance should always beat a less recent one.
To understand why this happens, it helps to know the basics of the integration set-up flow, which goes like this:
getIntegrationsToSetupfunction, which returns an array of integrations to installBecause of the last point, if
getIntegrationsToSetupreturns a list containing duplicates, unexpected things can happen. For example, consider the followinggetIntegrationsToSetupreturn values under the current implementation:[UserProvidedCopyofIntegration, DefaultCopyOfSameIntegration]- the user's copy should win, and does[DefaultCopyofIntegration, UserProvidedCopyOfSameIntegration]- the user's copy should win, but doesn't[DefaultCopyAofIntegration, DefaultCopyBofIntegration]- copy B should win, but doesn't[UserCopyAofIntegration, UserCopyBofIntegration]- copy B should win, but doesn'tThe most straightforward way to fix this would be to make it so that installing an existing integration would overwrite the existing copy with the new copy, but that would change the end result not just in the above situations (which all involve a single
Sentry.init()call) but also in situations involving competingSentry.init()calls and in situations where a second (third, fourth, etc) client or hub is being initialized directly. In those latter cases, we do want the first copy installed to take precedence, because it presumably corresponds to the "main"Sentryinstance.In order not to cause that larger behavior shift, it's therefore better to fix the aforementinoed bugs by making sure that a)
getIntegrationsToSetupnever returns duplicates, and b) its deduping process preserves the correct copy of each integration.This both makes that change and introduces a new (hopefully more comprehensive) test suite.
The roots of the problem in the current code are:
To solve this, the following changes have been made:
Note that in order to show that this change doesn't break existing behavior, for the moment the original tests (other than the one explicitly testing wrong behavior) have been retained for now. Since the new tests cover everything the existing tests cover, however, the existing tests can (and will) be removed in the future.