feat(node-experimental): Add `@sentry/node-experimental` package as MVP for POTEL by mydea · Pull Request #8609 · getsentry/sentry-javascript

mydea · 2023-07-21T12:06:40Z

This introduces a new package @sentry/node-experimental, which is an MVP implementation for Performance powered by OTEL (POTEL).

This package provides a wrapper over @sentry/node (also using @sentry/opentelemetry-node) which uses opentelemetry for performance instrumentation under the hood.

This is all abstracted away from the user, they only need to do:

import * as Sentry from '@sentry/node-experimental';

Sentry.init({
  dsn: 'my-dsn',
  tracesSampleRate: 0.1
});

And it will set up all the necessary integrations etc. for the user, including the default integrations + any applying performance integrations.

For this, I added all performance integrations that exist for node, minus undici (which will take some more work to get going), plus some new stuff: mysql, fastify, nest. Also, mongoose is it's own integration now, not handled via mongo anymore.

Note that I've only manually tested express & http so far. All others still need to be verified etc. But this is really a POC, which may go somewhere or not - by publishing this, it becomes super easy to try this in various scenarios.

I have tweaked the Http integration to create more or less the same output as we're used to for Sentry. I also made small tweaks to @sentry/opentelemetry-node to allow us to communicate with this a bit better. There are two main use cases I've identified there:

Tell the span processor to drop a span (=not send it to Sentry). For this, I added dedicated hooks.
Add some specific data/context/tags/metadata to an otel span, that should be added to the resulting sentry span. For this, I added utils which keep a map of data to be added in spanEnd.

mydea · 2023-07-21T12:08:10Z

.craft.yml

  ## 1.1 Types
  - name: npm
-    id: "@sentry/types"
+    id: '@sentry/types'


Prettier changed this for me 😅 Hope that's OK...

Ahh yes, when I worked on this, my prettier/vscode settings were broken and apparently our lint check didn't catch it. Should be fine.

mydea · 2023-07-21T12:45:58Z

Q: We could also publish this as @sentry-internal/node-experimental? 🤔 WDYT?

github-actions · 2023-07-24T11:31:03Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	75.07 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	31.4 KB (0%)
@sentry/browser - Webpack (gzipped)	21.95 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	69.79 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	28.28 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped)	20.33 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	220.89 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	85.99 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	60.48 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	30.41 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	65.14 KB (0%)
@sentry/react - Webpack (gzipped)	21.96 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	92.91 KB (0%)
@sentry/nextjs Client - Webpack (gzipped)	50.89 KB (0%)

Lms24

Nice, LGTM from a higher-level perspective. I only have limited context on the otel-specific parts but I like the overall concept of "just" wrapping the otel instrumentation with our integration class.

Lms24 · 2023-07-24T11:15:55Z

.craft.yml

  ## 1.1 Types
  - name: npm
-    id: "@sentry/types"
+    id: '@sentry/types'


Ahh yes, when I worked on this, my prettier/vscode settings were broken and apparently our lint check didn't catch it. Should be fine.

.craft.yml

Lms24 · 2023-07-24T11:20:27Z

docs/new-sdk-release-checklist.md

    ```yml
    - name: npm
-      id: npm:@sentry/[yourPackage]
+      id: '@sentry/[yourPackage]'


good catch, I forgot to adjust this

packages/node-experimental/LICENSE

Lms24 · 2023-07-24T11:27:25Z

packages/node-experimental/src/integrations/getAutoPerformanceIntegrations.ts

+import type { Postgres } from './postgres';
+import type { Prisma } from './prisma';
+
+const INTEGRATIONS = [


l (no action required):
I wonder if this is a good idea. The requires prolong startup time and that's an issue for serverless environments. But as I said, no action required at this time, we can revisit this later.

You mean to use this by default? The way this works I copied from the node auto discovery, only there it is opt-in via Sentry.autoDiscoverNodePerformanceMonitoringIntegrations().

With this setup, if you want to/need to avoid this, you can disable default integrations and opt-in to just the ones you want. Not sure if that is the best approach 🤔

Yes, I was concerned about making this opt-in. iirc we discussed dropping autoDiscoverNodePerformanceMonitoringIntegrations entirely in v7 because of the performance implications but decided to make it opt-in. I don't want to block this PR though so it's fine as is for now. Just wanted to bring this up.

Yeah, we can def. re-evaluate this, for now this is really experimental, and this specific part is easily changed later :) But let's keep this in mind! 👍

Lms24 · 2023-07-24T11:30:47Z

packages/node-experimental/src/integrations/http.ts

+
+    addOtelSpanData(span.spanContext().spanId, additionalData);
+
+    getCurrentHub().addBreadcrumb(


m: do we need to check if the Breadcrumbs integration is configured to record http breadcrumbs?
(Not sure about this, I just know that I had to do this for client-side sveltekit fetch)

Hmm not sure, if I look at the hub in core it has this method always 🤔 that delegates to scope.addBreadcrumb, which also seems to always be defined.

Sorry, I mixed up the integrations, I rechecked and saw that the current Http integration has a breadcrumbs option which you added here as well.

but shouldn't we nevertheless guard this line with this._breadcrumbs? afaict we only check this._breadcrumbs at the beginning to early return if both, breadcrumbs and tracing are disabled.

hah, you're absolutely right of course, completely forgot this 😅 good catch, will add the guard!

packages/node-experimental/src/sdk/init.ts

packages/node-experimental/src/sdk/client.ts

Lms24

Q: We could also publish this as @sentry-internal/node-experimental? 🤔 WDYT?

I think the @sentry/ namespace is totally fine. We clearly mark it as experimental so I don't see a problem. But no strong opinions either, so ultimately whatever you prefer.

AbhiPrasad · 2023-07-24T16:06:02Z

packages/opentelemetry-node/src/hooks.ts

+   * Receives the Otel Span as argument.
+   * Return `false` from the callback to skip this span in Sentry.
+   */
+  public on(hook: 'spanEnd', callback: (otelSpan: OtelSpan, sentrySpan: Span) => void | false): void;


Why can't we just use hooks on the default client? We can just append with experimental

I'd like to avoid using a separate class if we can.

The idea here was because we need this to have a possible return value, which we don't support right now (for a reason, makes stuff easier). Also I wanted to avoid adding an experimental hook because it is kind of public API then and we can't remove it again? but maybe that doesn't matter as well... 🤔

Thinking about this, one API that could work is:

const mutableOptions = { drop: false }; client.emit('otelSpanEnd', otelSpan, mutableOptions); if (mutableOptions.drop) { // drop this span } // And at hook register site client.on('otelSpanEnd', (otelSpan, mutableOptions) => { if (shouldDropSpan(otelSpan) { mutableOptions.drop = true; } });

WDYT about this?

I changed it to use otelSpanEnd hook on the general client!

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>

mydea requested review from AbhiPrasad, Lms24 and lforst July 21, 2023 12:06

mydea self-assigned this Jul 21, 2023

mydea commented Jul 21, 2023

View reviewed changes

mydea force-pushed the fn/node-preview-otel branch from 0da5946 to ae4abfb Compare July 21, 2023 12:08

Lms24 approved these changes Jul 24, 2023

View reviewed changes

mydea force-pushed the fn/node-preview-otel branch from dc141b9 to 419629d Compare July 24, 2023 12:16

Lms24 reviewed Jul 24, 2023

View reviewed changes

AbhiPrasad reviewed Jul 24, 2023

View reviewed changes

mydea force-pushed the fn/node-preview-otel branch from 094c28e to 74900c0 Compare July 25, 2023 11:16

mydea marked this pull request as ready for review July 25, 2023 12:08

mydea and others added 10 commits July 31, 2023 09:35

feat(node-experimental): MVP for POTEL

d2c2005

fix otel tests

274423c

skip tests for old node

d3e393b

Apply suggestions from code review

3db7c49

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>

fix craft oder

7a9188c

rename to NodeExperimental*

31a2fcd

add sdk metadata

6c6f867

fix verdaccio config

9291bd3

check for breadcrumbs setting

dd34b8f

ref: Use client hooks

4319909

mydea force-pushed the fn/node-preview-otel branch from 74900c0 to 4319909 Compare July 31, 2023 07:36

mydea merged commit a858a07 into develop Jul 31, 2023

mydea deleted the fn/node-preview-otel branch July 31, 2023 08:04


		addOtelSpanData(span.spanContext().spanId, additionalData);

		getCurrentHub().addBreadcrumb(

Uh oh!

Conversation

mydea commented Jul 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea commented Jul 21, 2023

Uh oh!

github-actions bot commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jul 24, 2023 •

edited

Loading

Lms24 Jul 24, 2023 •

edited

Loading