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

SDK internal errors being raised as events in Sentry. #9760

Closed
3 tasks done
rodolfoBee opened this issue Dec 6, 2023 · 8 comments
Closed
3 tasks done

SDK internal errors being raised as events in Sentry. #9760

rodolfoBee opened this issue Dec 6, 2023 · 8 comments
Labels
Package: replay Issues related to the Sentry Replay SDK Stale Type: Bug

Comments

@rodolfoBee
Copy link
Member

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/react

SDK Version

7.75.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

Sentry.init({
   environment: env,
   dsn: "...",
   integrations: [
      new Sentry.BrowserTracing({
         tracePropagationTargets: [...],
      }),
      new Sentry.Replay(),
   ],

   tracesSampleRate: 1.0, 
   replaysSessionSampleRate: 0.001, 
   replaysOnErrorSampleRate: 1.0, 
   allowUrls: [...],
   ignoreErrors: [....],
});

Steps to Reproduce

The SDK is reaching the replay buffer limit (source code) when recording session replays. I was not able to reproduce it on my own account.

This is raising an event sent by the SDK to the project configured in the SDK initialisation:

Error
Event buffer exceeded maximum size of 20000000.

The current workaround for affected users is to filter these events with ignoreErrors:

Sentry.init({
  dsn: "...",
  ignoreErrors: ["Event buffer exceeded maximum size"],
  ...
});

Originally reported here

Expected Result

The SDK should gracefully drop the event exceeding buffer and not raise an exception captured by the unhandled rejection handler.

Actual Result

An event is sent to the Sentry project as in the screenshot (not adding link due to privacy):
Screenshot 2023-12-06 at 10 47 01

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Dec 6, 2023
@github-actions github-actions bot added the Package: react Issues related to the Sentry React SDK label Dec 6, 2023
@lforst lforst added the Package: replay Issues related to the Sentry Replay SDK label Dec 6, 2023
@Lms24 Lms24 removed the Package: react Issues related to the Sentry React SDK label Dec 7, 2023
@Lms24
Copy link
Member

Lms24 commented Dec 7, 2023

I took a look at the bundled javascript code linked in the Sentry issue (from zendesk).

I'm not sure if this is really it but what we can see in the bundled code is that the minified version of _addEvent is still an async function but the call to replay.eventBuffer.addEvent is no longer awaited. This causes the try/catch block to no longer catch the error:

async function as(t, e, n) {
        if (!t.eventBuffer) return null;
        try {
          n && "buffer" === t.recordingMode && t.eventBuffer.clear(),
            n && (t.eventBuffer.hasCheckout = !0);
          var i = (function (t, e) {
            try {
              if ("function" == typeof e && t.type === xr.Custom) return e(t);
            } catch (t) {
              return (
                ("undefined" != typeof __SENTRY_DEBUG__ && !__SENTRY_DEBUG__) ||
                  _.c.error(
                    "[Replay] An error occured in the `beforeAddRecordingEvent` callback, skipping the event...",
                    t,
                  ),
                null
              );
            }
            return t;
          })(e, t.getOptions().beforeAddRecordingEvent);
          return i ? t.eventBuffer.addEvent(i) : void 0; // <-- no await here
        } catch (e) {
          const n = e && e instanceof $o ? "addEventSizeExceeded" : "addEvent";
          ("undefined" != typeof __SENTRY_DEBUG__ && !__SENTRY_DEBUG__) ||
            _.c.error(e),
            await t.stop({ reason: n });
          const r = U().getClient();
          r && r.recordDroppedEvent("internal_sdk_error", "replay");
        }
      }

any chance that the customer is using some kind of bundling plugin that replaces return await statements with return? Usually that'd be fine but not inside try/catch blocks.

@Lms24
Copy link
Member

Lms24 commented Dec 7, 2023

Fwiw, I also checked our transpilation output and in both CJS and ESM, we're still return awaiting the call.

@jes490
Copy link

jes490 commented Jan 15, 2024

I think it happens in the EventBufferProxy._switchToCompressionWorker. You can see a call for this._compression.addEvent(event) without try/catch here, so Promise.reject inside EventBufferCompressionWorker.addEvent fires an exception, and because it is a sync function (not async), it doesn't get caught inside catch in the _switchToCompressionWorker.

   async _switchToCompressionWorker() {
    const { events, hasCheckout } = this._fallback;

    const addEventPromises = [];
    for (const event of events) {
      addEventPromises.push(this._compression.addEvent(event));
    }

    this._compression.hasCheckout = hasCheckout;

    // We switch over to the new buffer immediately - any further events will be added
    // after the previously buffered ones
    this._used = this._compression;

    // Wait for original events to be re-added before resolving
    try {
      await Promise.all(addEventPromises);
    } catch (error) {
      (typeof __SENTRY_DEBUG__ === 'undefined' || __SENTRY_DEBUG__) && utils.logger.warn('[Replay] Failed to add events when switching buffers.', error);
    }
  }

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 15, 2024
@mydea
Copy link
Member

mydea commented Jan 15, 2024

I think it happens in the EventBufferProxy._switchToCompressionWorker. You can see a call for this._compression.addEvent(event) without try/catch here, so Promise.reject inside EventBufferCompressionWorker.addEvent fires an exception, and because it is a sync function (not async), it doesn't get caught inside catch in the _switchToCompressionWorker.

   async _switchToCompressionWorker() {
    const { events, hasCheckout } = this._fallback;

    const addEventPromises = [];
    for (const event of events) {
      addEventPromises.push(this._compression.addEvent(event));
    }

    this._compression.hasCheckout = hasCheckout;

    // We switch over to the new buffer immediately - any further events will be added
    // after the previously buffered ones
    this._used = this._compression;

    // Wait for original events to be re-added before resolving
    try {
      await Promise.all(addEventPromises);
    } catch (error) {
      (typeof __SENTRY_DEBUG__ === 'undefined' || __SENTRY_DEBUG__) && utils.logger.warn('[Replay] Failed to add events when switching buffers.', error);
    }
  }

This code looks correct to me - we are returning a rejecting promise from addEvent(). And this code:

await Promise.all(addEventPromises);

should catch it if any of these promises reject 🤔

If you enable debug mode for the SDK (Sentry.init({ debug: true, ... })), do you see this breadcrumb/log somewhere:

[Replay] Failed to add events when switching buffers.

?

@jes490
Copy link

jes490 commented Jan 15, 2024

@mydea Sorry, you're actually right, I was in a hurry when I checked this and completly missed the "return" statement for Promise.reject().

I revisited the issue and the part where it fails is still in EventBufferCompressionWorker, here's stacktrace:
image

But this code seems ok as well. Not sure what's the reason. I'll check a transpiled bundle a bit later and tell the results.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 15, 2024
@getsantry getsantry bot moved this to Waiting for: Community in GitHub Issues with 👀 2 Jan 16, 2024
@Lms24
Copy link
Member

Lms24 commented Jan 16, 2024

@jes490 Looking at the bottom of your stack trace, I can see that this seems to be an older version of the (Replay) SDK. We changed this logic a while ago in #8938. Specifically, the void addEvent call (which seemed suspicious to me which is why I wanted to looked it up) was changed in the PR. I'd suggest updating to the most recent version of the Sentry SDK you're using to check if the error still persists. Please note that all @sentry/* packages have to have the same version.

Let us know if this fixes things for you.

@getsantry
Copy link

getsantry bot commented Feb 7, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Feb 7, 2024
@getsantry getsantry bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 15, 2024
@demc
Copy link

demc commented Jun 26, 2024

@Lms24

I am running into this exact issue on v8.3.0.

Error: Event buffer exceeded maximum size of 20000000.
  at EventBufferCompressionWorker.addEvent(./node_modules/@sentry-internal/replay/esm/index.js:5140:1)
  at EventBufferProxy.addEvent(./node_modules/@sentry-internal/replay/esm/index.js:5246:1)
  at _addEvent(./node_modules/@sentry-internal/replay/esm/index.js:5649:1)
  at addEventSync(./node_modules/@sentry-internal/replay/esm/index.js:5600:1)
  at cb(./node_modules/@sentry-internal/replay/esm/index.js:7224:1)
  at ReplayContainer.addUpdate(./node_modules/@sentry-internal/replay/esm/index.js:8138:1)
  at this._stopRecordingemit(./node_modules/@sentry-internal/replay/esm/index.js:7212:1)
  at value(./node_modules/@sentry/utils/esm/buildPolyfills/_optionalChain.js:28:1)
  at fn(./node_modules/@sentry-internal/replay/esm/index.js:3612:27)
  at _optionalChain(./node_modules/@sentry/utils/esm/buildPolyfills/_optionalChain.js:28:1)
  at wrappedEmit(./node_modules/@sentry-internal/replay/esm/index.js:3612:13)
  at wrappedMutationEmit(./node_modules/@sentry-internal/replay/esm/index.js:3643:1)
  at this.emit(./node_modules/@sentry-internal/replay/esm/index.js:1949:1)
  at this.processMutations(./node_modules/@sentry-internal/replay/esm/index.js:1752:1)
  at observer(./node_modules/@sentry-internal/replay/esm/index.js:2289:1)
  at MutationObserver.<anonymous>(./node_modules/@sentry-internal/replay/esm/index.js:2243:1)

Is the advised workaround to add ignoreErrors: ["Event buffer exceeded maximum size"] to our init?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK Stale Type: Bug
Projects
Archived in project
Archived in project
Development

No branches or pull requests

6 participants