getsentry / sentry-react-native

Official Sentry SDK for React Native
https://sentry.io
MIT License
1.58k stars 337 forks source link

Duplicate http / xhr breadcrumbs #3045

Open AntoineThibi opened 1 year ago

AntoineThibi commented 1 year ago

OS:

Platform:

SDK:

SDK version: 4.13.0

react-native version: 0.71.7

Are you using Expo?

Are you using sentry.io or on-premise?

If you are using sentry.io, please post a link to your issue so we can take a look:

[Link to issue]

Configuration:

(@sentry/react-native)

Sentry.init({
  dsn: 'https://...@sentry.io/...'
  enableInExpoDevelopment: true,
  tracesSampleRate: 1,
  normalizeDepth: 10,
  integrations: [
       new Sentry.Native.ReactNativeTracing({
          routingInstrumentation,
          tracePropagationTargets: [appEnv.apiUrl],
        }),
    ],
});

I have following issue:

When I have a natif crash, the breadcrumbs for the http request appears 3 times : Capture d’écran 2023-05-05 à 10 16 10

After looking into it, I have the impression that the breadcrumbs of category "http" are coming from the native : if I look into the beforeBreadcrumb in the Sentry.init (in JS), I can't find those breadcrumbs. I only find the "xhr" breadcrumb.

Steps to reproduce:

Actual result:

In the error, I have 3 times the same call API. If I filter my breadcrumbs with beforeBreadcrumbs, only the xhr will be affected.

Capture d’écran 2023-05-05 à 10 16 10

Expected result:

There should be only one log of the API call. It should be affected by the beforeBreadcrumbs in the Sentry.init.

krystofwoldrich commented 1 year ago

Hi, thank you for the message, this looks like a bug, we are looking into it.

This might be related to a sentry-cocoa issue.. Plus one duplicate is from the RN layer.

krystofwoldrich commented 1 year ago

I've tested this and the SDK is creating duplicate breadcrumbs.

krystofwoldrich commented 1 year ago

SentryNetworkTracker from sentry-cocoa gets executed on xhr and fetch requests from the RN JS layer.

Tested on the sample-new-arch with RN 0.71.

JS network requests are executed as NSURLRequest in RN on iOS.

marandaneto commented 1 year ago

Good catch @krystofwoldrich so at the end, fetch forwards to iOS native, and then Sentry swizzling is able to detect it. The fix is more complex tho because we want breadcrumbs for JS and Native but also not duplicating it. An alternative is to not log fetch crumbs for iOS when native is enabled.

krystofwoldrich commented 1 year ago

I see multiple solutions each with different drawbacks ~(from most favorable to least)~:

~1. Only create breadcrumbs in JS (works with before-breadcrumb, lose native requests - not initiated from JS)~ ~2. Only create breadcrumbs in Obj-C (doesn't work with before-breadcrumbs, keeps all requests)~

  1. Don't create breadcrumbs on native for Requests from JS (works with before-breadcrumb, keeps all requests, requires marking the request from JS for example extra header that will be removed by sentry-cocoa interceptor).
  2. Filter duplicates (it might not be possible to recognize duplicates and consecutive calls)
GriffinSSloves commented 1 year ago

Is this still in consideration for being worked on? The native breadcrumbs are unable to be filtered out using the beforeBreadcrumb hook.

My app makes a lot of minor network requests which aren't significant enough to be a breadcrumb. But without the filtering in beforeBreadcrumb, there's no way to prevent them from going into the breadcrumb stream and crowding out the real breadcrumbs that would be helpful for debugging.

Thanks for looking into it!

krystofwoldrich commented 1 year ago

@GriffinSSloves Hi, yes this is still on our backlog.

Currently, you can remove the native breadcrumbs in beforeSend. It's a workaround, the breadcrumbs will still be created but they won't show in Sentry.

Sentry.init({
  beforeSend: (event: Sentry.Event) => {
    event.breadcrumbs = event.breadcrumbs?.filter(
      // Add your condition
      breadcrumb => breadcrumb.category !== 'http',
    );
    return event;
  },
};
LonelyCpp commented 5 months ago

probably better to use beforeBreadcrumb. So that it won't occupy the breadcrumb stack

beforeBreadcrumb: (breadcrumb: Sentry.Breadcrumb) => {
      console.log('sentry-event beforeBreadcrumb:', breadcrumb);

      // remove automatic http breadcrumbs (from native)
      const isHttp = breadcrumb.category === 'http';

      if (!isHttp) {
        return breadcrumb;
      }

      return null;
    },
krystofwoldrich commented 5 months ago

@LonelyCpp The beforeBreadcrumb callback is not executed for the native breadcrumbs.

They are added here -> https://github.com/getsentry/sentry-react-native/blob/5e2c81c3cb95db62ff47dc0dd2d841b5a744b7d5/src/js/integrations/devicecontext.ts#L94-L99