@sentry/browser - Breadcrumbs Integration click handler falsely implemented?

[sod]

Package + Version

• @sentry/browser

Version:

5.4.1

Description

I currently investigate why our page is slow and found a lot of clearTimeout, setTimeout calls.

There is some pretty weired behavior regarding @sentry/browser and I'm not really sure if this is supposed to be:

Reproduction, just go on any page that has @sentry/browser installed and type in the console:

debugger; document.body.addEventListener('click', () => 1)

Then watch in the debugger what sentry does. It invokes the breadcrumbEventHandler in

https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/integrations/breadcrumbs.ts#L163-L165

And then immediately invokes it with the element itself:

This doesn't seem right? Shouldn't this method be called on click and not immediately?

On a page of ours we register ~120 click handler, so this methods debounce is constantly calling setTimeout/clearTimout (that is again shimmed by sentry itself + in our case zone.js/angular) so the call stack and overhead is kinda huge.

[sod]

When I comment out the line breadcrumbEventHandler('click', true)(this); it cuts the rendertime of the angular8 page in half (from 1 second to 0.5 seconds)

[sod]

Found another issue the breadcrumbs introduce. As soon as you use the Breadcrumb integration, every request now runs 4 global application.tick instead of only one when the request finishes, because the Breadcrumb fire an even onreadystatechange, and onreadystatechange event always fires 3 times before the request is done.

[namelessvoid]

As mentioned in the first post, there is definitvely a bug here: https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/integrations/breadcrumbs.ts#L175

As soon as a click (and probably keypressed) event listener is added via el.addEventListener('click', ...), sentry creates a breadcrumb. I created a minimal working example via react at: https://github.com/namelessvoid/sentry-breadcrumb-issue

Set your sentry DSN in the index.js file. Then, when clicking "Trigger error", a unhandled error is thrown and caught by sentry. When viewing the error in sentry, you also see a ui.click breadcrumb on the div element which never happened (the first ui.click breadcrumb):

If you add more than one event handler, the debouncing seems to limit the number of erroneously generated breadcrumbs to a single one.

[rhcarvalho]

From #2317 (comment), this depends on #2299, which is still open.

[rhcarvalho]

FWIW, this comment from @lobsterkatie with a workaround might be relevant for anyone landing here: #2299 (comment)

[lobsterkatie]

To be clear, that workaround turns off error handling and keeps breadcrumb recording, but it could be adapted to do the reverse (if the goal is to turn off breadcrumb recording entirely).

[laukstein]

This might be the reason why Sentry broke click and other events #2130. Likely this approach is wrapping also another events, and if my code uses a timer to trigger or not events (for example to accept "click" only if executed after less than 100 ms after "pointerdown" event), it will fail do to this Sentry setTimeout.

[WolfspiritM]

Having the same issue. addEventListener always creates a wrong "ui.click" event. It's exactly the same issue as #2197

Not sure if that's related but I also noticed that if an exception is thrown inside a click handler then sentry/browser doesn't send this as a breadcrumb as the breadcrumb seams to be created AFTER the event is send.
That makes the breadcrumbs not very helpful in general...

[jdelStrother]

I've been wondering for a while why it seemed like every single Sentry-reported issue seemed to involve the user clicking on a search button:

Turns out that it's just that we're calling searchButton.addEventListener("click", handler, false) on page load, which always adds that breadcrumb.

I'm bemused that this bug has persisted for 17 months without a fix 😕

[micahjon]

Wow, hard to believe this bug still exists.

Here's a temporary workaround we added in the Sentry.init method -- simply checking to see if the hint.event property is an instance of a DOM Element instead of the Event class.

beforeBreadcrumb(breadcrumb, hint) {
    // Ignore "ui.click" breadcrumbs that result from "click" event listeners being added
    // https://github.com/getsentry/sentry-javascript/issues/2125
    if (breadcrumb.category === 'ui.click' && hint && hint.event instanceof Element) {
        return null;
    }

    return breadcrumb;
},

[laukstein]

@micahjon, Sentry breaks click events #2130 and it was the reason me to moved to @bugsnag/bugsnag-js.

[jordie23]

Just spent a few days trying to track down this issue cause I thought I was going crazy.

Thanks for the work around @micahjon - disappointing that a work around is required to have sentry provide accurate information.

Edit: To add more info, I'm using nextjs (so react). It appears I'm getting breadcrumbs come through when click events are attached, instead of only when they're triggered.

[laukstein]

Can anyone verify #3208 actually fixes #2125 and #2130?

[micahjon]

@laukstein , as far as I can tell it does fix this issue (#2125)