feat(node-experimental): Add `@sentry/node-experimental` package as MVP for POTEL #8609

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

@@ -6,101 +6,104 @@ targets:
  ## 1. Base Packages, node or browser SDKs depend on
  ## 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

@@ -6,101 +6,104 @@ targets:
  ## 1. Base Packages, node or browser SDKs depend on
  ## 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

@@ -61,7 +61,7 @@ When you’re ready to make the first release, there are a couple of steps that
 - [ ]  3) Add an `npm` target in `craft.yml` for the new package. Make sure to insert it in the right place, after all the Sentry dependencies of your package but before packages that depend on your new package (if applicable).
    ```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 lforst, Lms24 and AbhiPrasad 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node-experimental): Add `@sentry/node-experimental` package as MVP for POTEL #8609

feat(node-experimental): Add `@sentry/node-experimental` package as MVP for POTEL #8609

mydea commented Jul 21, 2023

mydea Jul 21, 2023

Lms24 Jul 24, 2023

mydea commented Jul 21, 2023

github-actions bot commented Jul 24, 2023 •

edited

Loading

Lms24 left a comment

Lms24 Jul 24, 2023

Lms24 Jul 24, 2023

Lms24 Jul 24, 2023

mydea Jul 24, 2023

Lms24 Jul 24, 2023

mydea Jul 24, 2023

Lms24 Jul 24, 2023 •

edited

Loading

mydea Jul 24, 2023

Lms24 Jul 24, 2023

mydea Jul 24, 2023

Lms24 left a comment

AbhiPrasad Jul 24, 2023

mydea Jul 24, 2023

mydea Jul 25, 2023


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

		getCurrentHub().addBreadcrumb(

feat(node-experimental): Add @sentry/node-experimental package as MVP for POTEL #8609

feat(node-experimental): Add @sentry/node-experimental package as MVP for POTEL #8609

Conversation

mydea commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea commented Jul 21, 2023

github-actions bot commented Jul 24, 2023 • edited Loading

size-limit report 📦

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(node-experimental): Add `@sentry/node-experimental` package as MVP for POTEL #8609

feat(node-experimental): Add `@sentry/node-experimental` package as MVP for POTEL #8609

github-actions bot commented Jul 24, 2023 •

edited

Loading

Lms24 Jul 24, 2023 •

edited

Loading