Performance Issue for NestJS Project with Sentry Performance Enabled

[ReneGreen27]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/browser

SDK Version

9.5.0

Framework Version

NestJS

Link to Sentry event

No response

Reproduction Example/SDK Setup

Mini repro app and details regarding performance included in internal ticket #146790

Steps to Reproduce

clean NestJS framework + Sentry. The route for testing simply returned a small JSON text.

According to the measurement results:

Clean framework without Sentry - 30,000 rps, 3ms average response time.

Clean framework + Sentry - 2,500 rps, 39ms average response time.

I used the following Sentry configuration:

Sentry.init({
  dsn: '',
  defaultIntegrations: false,
  integrations: [
  nodeProfilingIntegration(),
  Sentry.nestIntegration(),
  Sentry.httpIntegration(),
  Sentry.onUncaughtExceptionIntegration(),
  Sentry.onUnhandledRejectionIntegration(),
  ],
  environment: 'local',
  tracesSampleRate: 1,
  profilesSampleRate: 1,
});

Reducing the values of tracesSampleRate and profilesSampleRate to 0.1 slightly improves the situation

main performance loss occurs due to the Sentry.httpIntegration() module

Expected Result

Not impactful to app performance to have sentry performance enabled

Actual Result

Performance affected

[andreiborza]

Thanks for the easy to follow reproduction repo. Can confirm, I'm observing the same outcomes with the http integration added as the only integration. I also tried production mode with the same results.

When only using the httpIntegration and disabling everything that's configurable inside it, I got up to 27k rps and 3ms avg response time on my machine. As a baseline I'm getting 85k rps tho.

Sentry.httpIntegration({
   breadcrumbs: false,
   spans: false,
   trackIncomingRequestsAsSessions: false,
   disableIncomingRequestSpans: true,
}),

cc @chargome @mydea

[chargome]

Hey @ReneGreen27, we have several PRs in place that should slightly improve performance, but generally the instrumentation will cause some overhead. Current recommendation is to use the setup posted above by @andreiborza and update to latest versions as we'll try to further optimize this.

[lforst]

Here's a flamegraph of an extremely simple node.js server load tested:

https://drive.google.com/file/d/19CC_zFcRhVme_exz1gpthNv6fh8tNcKe/view?usp=sharing

const Sentry = require("@sentry/node");

Sentry.init({
  dsn: "....",

  // Add Tracing by setting tracesSampleRate
  // We recommend adjusting this value in production
  tracesSampleRate: 1.0,

  // Set sampling rate for profiling
  // This is relative to tracesSampleRate
  profilesSampleRate: 1.0,

  debug: true,

  environment: "node-test-app",
});

const http = require("http");

const hostname = "127.0.0.1";
const port = 3000;

const server = http.createServer((req, res) => {
  res.statusCode = 200;
  res.setHeader("Content-Type", "text/plain");
  res.end("Hello, World!\n");
});

server.listen(port, hostname, () => {
  console.log(`Server running at http://${hostname}:${port}/`);
});

Some offenders I could make out:

• httpRequestToRequestData - or maybe drop undefined keys
• processEvent
• promiseBuffer is using an enormous amount of self-time
• normalize
• otel’s getIncomingRequestAttributes
• for whatever reason some anonymous function in the SentryHttpInstrumentation
• processEvent in eventFilters.js
• createTransactionForOtelSpan
• I think logger we can ignore, I had debug: true in the test

[AbhiPrasad]

Found another one - looks like the spanStart hooks we run in integrations can be avoided

I know we're improving dropUndefinedKeys but we should look at if we can avoid adding client hooks if no instrumentation was added.

For example with tedious, we should not even register client.on('spanStart' if tedious is not instrumented (which for vast majority of people it wont be

sentry-javascript/packages/node/src/integrations/tracing/tedious.ts

Lines 27 to 38 in 3d63621

	client.on('spanStart', span => {
	const { description, data } = spanToJSON(span);
	// Tedius integration always set a span name and `db.system` attribute to `mssql`.
	if (!description || data['db.system'] !== 'mssql') {
	return;
	}
	const operation = description.split(' ')[0] || '';
	if (TEDIUS_INSTRUMENTED_METHODS.has(operation)) {
	span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.tedious');
	}
	});

We should also avoid dropUndefinedKeys in spanToJson

sentry-javascript/packages/core/src/utils/spanUtils.ts

Lines 150 to 163 in 3d63621

	return dropUndefinedKeys({
	span_id,
	trace_id,
	data: attributes,
	description: name,
	parent_span_id: parentSpanId,
	start_timestamp: spanTimeInputToSeconds(startTime),
	// This is [0,0] by default in OTEL, in which case we want to interpret this as no end time
	timestamp: spanTimeInputToSeconds(endTime) || undefined,
	status: getStatusMessage(status),
	op: attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP],
	origin: attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] as SpanOrigin | undefined,
	links: convertSpanLinksForEnvelope(links),
	});

[AbhiPrasad]

Object.defineProperty is def expensive. We should aim to avoid this as much as possible. I actually see a lot of use cases where we can replace this with a weakmap instead.

addChildSpanToSpan, setContextOnScope, and setCapturedScopesOnSpan

[AbhiPrasad]

shouldSample

uuid4

Why is getCrypto getter so expensive???

[AbhiPrasad]

#15767

[AbhiPrasad]

Note: After profiling the changes we made in:

• perf(core): Add short-circuits to eventFilters integration #15752
• ref: Avoid some usage of dropUndefinedKeys() #15757
• feat(core): Optimize dropUndefinedKeys #15760
• perf(nestjs): Remove usage of addNonEnumerableProperty #15766

We seem to have made some improvements!

We've dropped dropUndefinedKey usage total time by 50%, isPojo has pretty much disappeared off the list,

Before:

After:

Now we gotta make spanToJson avoid dropUndefinedKeys, and fix SentryError usage:

[mydea]

So dropUndefinedKeys is completely going away 🎉 : #15796

[mydea]

I wonder what makes SentryError slow 🤔

[lforst]

I wonder what makes SentryError slow 🤔

AFAIK instantiating errors is actually quite expensive because the runtime has to compute a stack trace. That might be it.

[lforst]

Maybe if we threw an object with a certain shape instead it wouldn't be so expensive.

[mydea]

Improvements that have been made now:

• Stopped using dropUndefinedKeys altogether
• Ensure that Node Performance Instrumentation only registers on('spanStart') handlers when they are actually used
• Stopped using SentryError
• Short-circuits to event processing integration

[AbhiPrasad]

Took another detailed trace, we've made some good improvements:

Here's whats still left that is notable:

uuid4 is expensive, mainly from generateTraceId usage:

addNonEnumerableProperty - tracked with #15808

processEvent, and normalize visit but hard to tell why

createTransactionForOtelSpan

OTEL sdk-trace-base Tracer.startSpan

inferSpanData - should be fixed with work in #15767

OTEL sanitizeAttributes

createAndFinishSpanForOtelSpan

isValidTraceId in otel span utils

[AbhiPrasad]

Since we've started the initiative we've improved performance by 30-40%.

The next release will contain a majority of these improvements - will leave a comment when that is released.

For now going to close this issue. Further improvements will be tracked by #15861

Aside from upgrading the SDK, adjusting the sample rate will also have a lot of benefits performance-wise. We recommend using that as an avenue to get application performance to something you think is reasonable.