getsentry / sentry-react-native

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

Duplicate Breadcrumbs #1409

Closed abdullahizzuddiin closed 2 years ago

abdullahizzuddiin commented 3 years ago

Environment

How do you use Sentry? sentry.io

Which SDK and version?

Steps to Reproduce

const dsn = Config.SENTRY_DSN;
const debug = false;
const release = `${DeviceInfo.getBundleId()}@${versionName}@${versionCode}+codepush:${codepushDist}`;
const dist = `${versionName}.${codepushDist}`;
const environment = isProductionEnv ? 'production' : 'staging';
const sampleRate = debug ? 1 : 0.5;
const tracesSampleRate = 0.2;
const maxBreadcrumbs = 150; 

    Sentry.init({
        dsn,
        debug,
        release,
        dist,
        environment,
        sampleRate,
        tracesSampleRate,
        maxBreadcrumbs,
    });

Set those initialization and surfing on my app normally until I get some errors.

Expected Result

breadcrumb data is not logged twice.

Actual Result

Looking at Breadcrumb section, I found that each breadcrumb data logged twice. xhr breadcrumb is logged by default by Sentry SDK. btn_clicked and navigate is manually logged, I mean, my code that logged it. Both of them logged twice.

image

this is how I add a breadcrumb.

export function recordBreadcrumb(key, message, level = Sentry.Severity.Info) {
    Sentry.addBreadcrumb({
        category: key,
        message,
        level,
    });
}
...
function logButtonClicked(buttonOrFunctionName, value = {}){
   ...
   CrashLoggingUtils.recordBreadcrumb('btn_clicked', buttonOrFunctionName);
}

I've ensured that logButtonClicked() only called once a time.

jennmueng commented 3 years ago

Hmmm this is unusual, are you running with fast refresh enabled? I found that sometimes code could be initialized twice causing two instances when you fast-refresh.

abdullahizzuddiin commented 3 years ago

Strangely, this issue also happened on release app.

If the code initialized twice, the log printed on console/terminal should be also twice right?

I put aconsole.log() inside my recordBreadcrumb(), the log printed on console only once.

jennmueng commented 3 years ago

No, the log would actually show up once, but as Sentry is a series of class instances, an instance from an "older" version of the app could still be present in the runtime while the recordBreadcrumb you have already became dead. Not sure if this makes sense but it's something I've run into once before with fast-refreshing (not breadcrumbs like your issue but with something else). So I'm not entirely sure this is applicable—would need to investigate the code we have for breadcrumbs and see if that could be susceptible to what I mentioned, plus this probably won't happen in a release app.

Example: Think of it as like, let's say you have some global event handler, then you add a listener, listenerA to it. When the part of the code that attaches the listener refreshes, it does not remove listenerA from the global event handler, keeping the class instance around it alive although all other code is dead. It then adds listenerB to the global event handler as the same code is run again on a fast refresh, now when an event is emitted both listeners are called.

abdullahizzuddiin commented 3 years ago

What a great explanation. Thanks!

According to your hypothesis, if I do fast-refresh four times, the breadcrumbs will be logged four times too, right?

jennmueng commented 3 years ago

What a great explanation. Thanks!

According to your hypothesis, if I do fast-refresh four times, the breadcrumbs will be logged four times too, right?

Yes, however, if we use my example again, you would also need to make sure that the troublesome code (wherever the listener is registered in this case) is what gets refreshed. If you refresh some other code and the troublesome code is not refreshed it would not add another listener.

abdullahizzuddiin commented 3 years ago

I found something interesting, actually I don't know if this information is valuable or not.

I configure beforeSend attribute,

export function init() {
    Sentry.init({
        dsn,
        beforeSend: (event, hint) => {
            console.log('event', event);
            console.log('hint', hint);

            return event;
        },
        debug,
        release,
        dist,
        environment,
        sampleRate,
        tracesSampleRate,
        maxBreadcrumbs,
    });
}

on event in beforeSend(), I found the breadcrumb is not logged twice.

image

But on Sentry UI, it displayed twice image

bruno-garcia commented 3 years ago

Could you please link the issue in Sentry? I assume the JSON event in there has the duplicated breadcrumb too, correct?

abdullahizzuddiin commented 3 years ago

Here the issue link

Nope. event (inside beforeEvent) I printed to console doesn't have duplicated breadcrumbs.

abdullahizzuddiin commented 3 years ago

Hi folks, any update for this issue?

@bruno-garcia @jennmueng

ted-nz commented 3 years ago

I am also observing this issue of breadcrumbs appearing twice, latest sdk, RN 0.64.

The appearance of breadcrumbs with data in the sentry web UI are also inconsistent at times.

i.e. one of my breadcrumb have data that could look like { lastScreen: null }, sometimes, lastScreen would be omitted in the sentry UI, other times it would literally render lastScreen: null. In the case of these duplicated/double entry, usually one entry is of one form, the other being the other. Not sure if it is of any consequence in investigating this issue.

bruno-garcia commented 3 years ago

@jennmueng could this be related to scope sync? Where the JS layer has a crumb in the event, then we pass the envelope to Android which merges it? @marandaneto might now more

marandaneto commented 3 years ago

could be, breadcrumbs are merged so it's likely this

jennmueng commented 3 years ago

@bruno-garcia @marandaneto I agree with this hypothesis but for some reason I cannot reproduce it. In theory you would assume all events sent from the React Native layer, with scope sync enabled, through the Android layer would merge the breadcrumbs and cause duplicates...but this is not what happens when I send test events with the sample app.

ted-nz commented 3 years ago

In terms of reproducing, not sure if this helps, but I'm in middle of active development, so would have been going through many builds & releases, bumping the build version/number each time. Each build would create new release in sentry, I'd install over the top of the previous build number on the same device, re-test, rinse, repeat.

Also doesn't happy all the time, seem like if I force a native crash, I don't tend to get double ups, but using Sentry.captureException without the app crashing seem to reproduce it reliably.

listenzz commented 3 years ago

Found the same problem on Android Platform.

The next line fix it.

Sentry.init({
      beforeSend: event => {
        if (event.breadcrumbs && Platform.OS === 'android') {
          delete event.breadcrumbs
        }
        return event
      },

Maybe the reason is:

package io.sentry

// line 491
  private void sortBreadcrumbsByDate(
      final @NotNull SentryEvent event, final @NotNull Collection<Breadcrumb> breadcrumbs) {
    final List<Breadcrumb> sortedBreadcrumbs = event.getBreadcrumbs();

    if (!breadcrumbs.isEmpty()) {
      sortedBreadcrumbs.addAll(breadcrumbs);
      Collections.sort(sortedBreadcrumbs, sortBreadcrumbsByDate);
    }
  }

The sortedBreadcrumbs come from:

// package io.sentry.react;
@ReactMethod
public void captureEnvelope(String envelope, Promise promise) {

}

And the breadcrumbs come from:

// package io.sentry.react;
@ReactMethod
public void addBreadcrumb(final ReadableMap breadcrumb) {

}
bruno-garcia commented 3 years ago

We should add a work around until we change the bridge approach for Android. A quick work around can be to clear the crumbs in the JS layer before calling the Android bridge (not the iOS):

https://github.com/getsentry/sentry-react-native/blob/7ed3d39074172bb902ee55c09b814b5604b6fd3d/src/js/wrapper.ts#L53

Make sure to check if syncScope is enabled and mechanism.handled is true, because in that case, all crumbs are available on the Android layer and will be applied.

retyui commented 3 years ago

Issue still reproduce on Android

<Button
   title='test'
   onPress={()=>{
     const obj = {};

     Sentry.addBreadcrumb({        message: 'test',    });

     obj.testSentry();
   }}
/>
"react-native": "0.63.3",
"@sentry/react-native": "^2.6.2",

fix helped https://github.com/getsentry/sentry-react-native/issues/1409#issuecomment-841188736

TexxUkr commented 3 years ago

Possibly it should be reopened... We have breadcrumbs duplication for 2.5.1 where the issue should have been fixed. For Android every time app calls Sentry.captureMessage(message) I see a a message with duplicated breadcrubms of xhr and analytics types.

https://github.com/getsentry/sentry-react-native/issues/1409#issuecomment-841188736 helps to fix it.

marandaneto commented 3 years ago

@jennmueng please have a look at it

lucas-zimerman commented 2 years ago

https://github.com/getsentry/sentry-react-native/blob/a085ec4ef6e9506cca568d8841e1e1bc8dfb63a2/src/js/wrapper.ts#L108-L110 Messages get duplicated breadcrumbs because it doesn't have an exception so the above condition gets skipped. Perhaps we should also delete all the breadcrumbs if there isnt an exception or the exception was handled, or

 if (event.exception?.values?.[0]?.mechanism?.handled != false) { 
   event.breadcrumbs = []; 
 } 
marandaneto commented 2 years ago

@lucas-zimerman Makes sense, wanna open up a PR? :)

lucas-zimerman commented 2 years ago

@lucas-zimerman Makes sense, wanna open up a PR? :)

Sorry for the late response, I'd like to... At least I'll apply it on capacitor :D

dsychev80 commented 1 month ago

Hi, guys! We have the same problem with duplication of breadcrumbs on iOS. Looks like it send breadcrumbs when request started and received (this you can see on image from xcode, sentry in debug mode). Image Image