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

Reduce blocking main thread during span logic #14067

Closed
theguacamoleking opened this issue Oct 23, 2024 · 5 comments · Fixed by #14094
Closed

Reduce blocking main thread during span logic #14067

theguacamoleking opened this issue Oct 23, 2024 · 5 comments · Fixed by #14094
Labels
Package: node Issues related to the Sentry Node SDK Stale

Comments

@theguacamoleking
Copy link

theguacamoleking commented Oct 23, 2024

Problem Statement

Hi there, I wanted to flag an issue where my deeper GraphQL queries take exponentially longer when using Sentry. This looks to be because Sentry logic ends up blocking the main thread

Image

As you can see above, logic within @sentry/opentelemetry is taking up more than 50% of time on the main thread over the period of over 15s. With Sentry disabled, the same GraphQL request takes about 2 seconds.

Solution Brainstorm

System clock lookups

The following code is the worst offender

function shouldCleanupSpan(span, maxStartTimeOffsetSeconds) {
  const cutoff = Date.now() / 1000 - maxStartTimeOffsetSeconds;
  return core.spanTimeInputToSeconds(span.startTime) < cutoff;
}

Specifically the cutoff assignment. I'd propose something like the following where there is only one system clock read for each call of _cleanupOldSpans.

   _cleanupOldSpans(spans = this._finishedSpans) {
    // Get current time (once)
    const currentEpoch = Math.floor(Date.now() / 1000);

    this._finishedSpans = spans.filter(span => {
      const shouldDrop = core.spanTimeInputToSeconds(span.startTime) < currentEpoch - this._timeout);
      DEBUG_BUILD &&
        shouldDrop &&
        utils.logger.log(
          `SpanExporter dropping span ${span.name} (${
            span.spanContext().spanId
          }) because it is pending for more than 5 minutes.`,
        );
      return !shouldDrop;
    });
  }
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 23, 2024
@s1gr1d
Copy link
Member

s1gr1d commented Oct 24, 2024

Hello thanks for raising this and also digging into this problem! I think your proposed change looks reasonable. It's not needed to get a fresh timestamp on every filter. Are you interested in submitting a PR?

@AbhiPrasad
Copy link
Member

Opened a PR for this! #14094

@AbhiPrasad AbhiPrasad self-assigned this Oct 28, 2024
AbhiPrasad added a commit that referenced this issue Oct 28, 2024
resolves #14067

Avoid calling `Date.now()` for each span in the span exporter. This
should reduce blocking I/O.
Copy link
Contributor

A PR closing this issue has just been released 🚀

This issue was referenced by PR #14094, which was included in the 8.36.0 release.

@AbhiPrasad
Copy link
Member

Hi @theguacamoleking could you try the latest release of the JS SDK to see if the profiles look better? We've made some fixes since then.

@AbhiPrasad AbhiPrasad added Package: node Issues related to the Sentry Node SDK Waiting for: Community labels Dec 10, 2024
@getsantry getsantry bot moved this to Waiting for: Community in GitHub Issues with 👀 3 Dec 10, 2024
@getsantry
Copy link

getsantry bot commented Jan 1, 2025

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 Jan 1, 2025
@getsantry getsantry bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Stale
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants