Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetch doesn't show up in breadcrumbs if user has LastPass' Chrome extension installed #1601

Closed
teeberg opened this issue Sep 30, 2018 · 2 comments · Fixed by #2103
Closed
Assignees

Comments

@teeberg
Copy link

teeberg commented Sep 30, 2018

Package + Version

I'm using sentry-javascript 4.0.6 from the CDN.

Description

I am currently trying to migrate to sentry-javascript and noticed during the migration that the fetch breadcrumbs disappeared. It looks like #1520 stopped logging them if fetch was detected to be replaced.

Disabling my extensions one by one, I found that the LastPass Chrome extension seems to replace window.fetch but still ends up calling the native .fetch(), even though the check from #1520 fails now, since window.fetch.toString(), with lastpass installed, returns:

function(t,n){var o=r(n),u=e.apply(this,arguments);if(o){var c=function(t){s({requestID:o,statusCode:t&&t.status})};u.then(c).catch(c)}return u}

i.e. does not include "native". So now, fetch doesn't show up in the breadcrumbs anymore at all. Turning off LastPass makes them be captured, judging by them appearing in the UI.

@kamilogorek
Copy link
Contributor

kamilogorek commented Oct 1, 2018

I know it's not perfect, but it's a tradeoff we had to make. We cannot control source code of users apps, and we're not able to retrieve original fetch method declaration either.

You can go around this manually using custom integrations though:

import { init } from '@sentry/browser';

class CustomFetchIntegration {
  install() {
    // put code from this method here and remove native detection
    // https://github.com/getsentry/sentry-javascript/blob/72a8bce6e500068206674c76835ea7b4154872a4/packages/browser/src/integrations/breadcrumbs.ts#L185
  }
}

init({
  dsn: '__YOUR_DSN__',
  integrations: [
    new Sentry.Integrations.BreadcrumbIntegrations({ fetch: false }),
    new CustomFetchIntegration()
  ]
});

@teeberg
Copy link
Author

teeberg commented Oct 1, 2018

I understand the original fix in #1520 was supposed to fix it showing up twice, which is a bug. But I would argue that it not showing up at all is just as much a bug, so I personally would rather have it show up twice than not at all by default.

Thank you for the workaround! I'll think about whether I want to copy over all the code from the fetch instrumentation. I'd really prefer to use as much of your code as possible, in case you fix bugs there in future releases. Having to check whether it changed and copy it over on every release doesn't sound optimal.

Some suggestions I could think of that would make my life easier in that regard:

  • Maybe you could expose the whole fill(global, 'fetch') { ... } code after if (!supportsNativeFetch()) { ... } as its own function? then it would be easier to reuse
  • Or maybe expose two slightly different fetch instrumentations, one of which ignores a fetch that was replaced. That way users could choose for themselves whether they'd rather have duplicate breadcrumbs or no breadcrumbs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants