getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.97k stars 1.57k forks source link

Cannot change/override breadcrumbs integration options #1981

Closed d7ark closed 5 years ago

d7ark commented 5 years ago

Package + Version

Version:

5.0.2

Description

Simple demo: https://github.com/d7ark/sentry-breadcrumbs-issue (need to provide sentry dsn in index.js).

I cannot find way to override Breadcrumbs integration options. Way provided in documentation is not working.

integrations: [new Sentry.Integrations.Breadcrumbs({ console: false })].

Solution from example app is also not working.

I have a hunch this is because the defaultIntegrations are creating new Integrations.Breadcrumbs which adds event listeners with default options (everything true) and there's no way to remove those listeners. But Im not sure.

I've found a workaround using beforeBreadcrumb but I'd prefer to use intented way (Breadcrumbs integration with { dom: false }).

  // workaround using beforeBreadcrumb
  Sentry.init({
    beforeBreadcrumb: removeUiBreadcrumbs,
    dsn: SENTRY_DSN,
  });

  function removeUiBreadcrumbs(breadcrumb) {
    return breadcrumb.category.startsWith('ui.') ? null : breadcrumb;
  }
HazAT commented 5 years ago

Thanks for taking the time writing this excellent bug report, I am looking into this right now.

HazAT commented 5 years ago

found the issue, 5.0.4 will fix this https://github.com/getsentry/sentry-javascript/pull/1983

HazAT commented 5 years ago

OK, we found the issue, the fix is not as simple as we thought. We hope to fix it tomorrow with 5.0.5

d7ark commented 5 years ago

It's perfectly fine. Thank you for your work. I'll keep an eye out.

HazAT commented 5 years ago

@d7ark So the problem is that we are emitting a breadcrumb in the try/catch integration we always emit a breadcrumb in case of an error. So the breadcrumb integration itself is working correctly. We still haven't found a good solution for this since wrapping addEventListener twice is tricky.

So to confirm, if you also remove the TryCatch Integration it should not emit a breadcrumb anymore.

HazAT commented 5 years ago

The fix for this will be shipped in the new version today:

The way it works then is like this:

Sentry.init({ 
...
    integrations(integrations) {
      integrations = integrations.filter(
        integration => integration.name !== "Breadcrumbs"
      );
      return [
        ...integrations,
        new Sentry.Integrations.Breadcrumbs({ dom: false })
      ];
    },
...
  });
zeusstl commented 3 years ago

Hello,

SDK: Name | sentry.cocoa Version | 7.3.0

I receive this error implementing the recommendation above from @HazAT .

Sentry.Integrations.Breadcrumbs is not a constructor
    at integrations (App.tsx:83)
    at getIntegrationsToSetup (integration.js:29)
    at Object.setupIntegrations (integration.js:59)
    at ReactNativeClient.BaseClient.setupIntegrations (baseclient.js:155)
    at Hub.bindClient (hub.js:58)
    at initAndBind (sdk.js:19)
    at Object.init (sdk.js:74)
    at eval (App.tsx:65)
    at invokePassiveEffectCreate (ReactNativeRenderer-dev.js:19280)
    at Object.invokeGuardedCallbackProd (ReactNativeRenderer-dev.js:93)
...

I also tried the following and it gave the same error: (Reference: https://docs.sentry.io/platforms/javascript/configuration/integrations/default/#modifying-system-integrations)

Sentry.init({
    dsn: SENTRY_DSN,
    debug: false,
    environment: APP_ENV,
    release: version,
    integrations: [new Sentry.Integrations.Breadcrumbs({ console: false })],
});

I deactivate console logs in production and Sentry is reactivating them which causes the production react native app to get overwhelmed on real devices.

Can anyone advise on where to find more accurate/complete documentation for how to deactivate the console breadcrumbs? (Or if you just have an answer that you can post here, that would be greatly appreciated.)

kamilogorek commented 3 years ago

Can you show a larger piece of code that includes your imports?

zeusstl commented 3 years ago
import React, { useState, useEffect } from 'react';
import { View } from 'react-native';
import codePush from 'react-native-code-push';
import { MenuProvider } from 'react-native-popup-menu';
import { NativeRouter } from 'react-router-native';
import { Provider } from 'react-redux';
import { PersistGate } from 'redux-persist/integration/react';
import * as Sentry from '@sentry/react-native';
import {ThemeProvider} from 'styled-components/native';
import {useTheme} from 'src/config/theme';
import Router from 'src/ui/AppRouter';

Is there something I should be explicitly importing?

kamilogorek commented 3 years ago

Change Sentry.Integrations.Breadcrumbs to Sentry.BrowserIntegrations.Breadcrumbs and it should work - https://github.com/getsentry/sentry-react-native/blob/master/src/js/index.ts#L46

zeusstl commented 3 years ago

Thanks. That resolved it. In the meantime, I found this too which stopped the console logs from being shown in the breadcrumbs:

beforeBreadcrumb(breadcrumb, hint) {
    return (breadcrumb.category === "console" && env=="production") ? null : breadcrumb;
},

Wondering if there is an advantage to one over the other - beforeBreadcrumb vs the integrations solution. It seems like not even implementing the integration in the first place would be advantageous, right?

Any thoughts there?

kamilogorek commented 3 years ago

It seems like not even implementing the integration in the first place would be advantageous, right?

Correct. Disabling integration will prevent the wrapping of the API completely, which will make sure its not performing any unnecessary work.