feat(opentelemetry): Add new `@sentry/opentelemetry` package #9238

mydea · 2023-10-13T07:41:29Z

This extracts the code from node-experimental in a more reusable @sentry/opentelemetry package.

This package has no direct (otel) dependencies, but only peer dependencies. This means it can be used by anybody with an existing OTEL setup without messing with their dependencies etc.

In turn, node-experimental uses this package to implement a concrete and opinionated implementation of this for node.

This new package should also be usable for non-node OTEL implementations, and concisously does not include any node specific stuff.

mydea · 2023-10-13T07:42:16Z

packages/opentelemetry/src/index.ts

+ * @hidden
+ */
+const _INTERNAL = {
+  addOriginToSpan,


I want to keep this private until we decided on getsentry/rfcs#116, only then I'd like to expose the internal attribute names etc. somehow.

mydea · 2023-10-13T07:43:28Z

packages/opentelemetry/src/contextManager.ts

+ * const SentryContextManager = wrapContextManagerClass(AsyncLocalStorageContextManager);
+ * const contextManager = new SentryContextManager();
+ */
+export function wrapContextManagerClass<ContextManagerInstance extends ContextManager>(


This has been generalized so we can wrap any context manager with our sentry specific behavior. Really we don't care what context manager is used, this should also work with browser-specific context managers (e.g. the is a zonejs based context manager etc).

mydea · 2023-10-13T07:43:46Z

packages/opentelemetry/src/custom/client.ts

+ * const OpenTelemetryClient = getWrappedClientClass(NodeClient);
+ * const client = new OpenTelemetryClient(options);
+ */
+export function wrapClientClass<


This has been generalized so you can wrap any client, e.g. either a node or a browser client.

mydea · 2023-10-13T07:44:46Z

packages/opentelemetry/src/semanticAttributes.ts

+ * These are internal and are not supposed to be used/depended on by external parties.
+ * No guarantees apply to these attributes, and the may change/disappear at any time.
+ */
+export const InternalSentrySemanticAttributes = {


Instead of constants, keeping this similar to how OTEL handles their semantic attributes. Eventually we should align this with getsentry/rfcs#116.

mydea · 2023-10-13T07:50:40Z

packages/opentelemetry/src/trace.ts

 */
-export function startSpan<T>(spanContext: NodeExperimentalSpanContext, callback: (span: Span | undefined) => T): T {
+export function startSpan<T>(spanContext: OpenTelemetrySpanContext, callback: (span: Span) => T): T {


Note: The API here has changed!! to what it was before.

Previously, the callback received span: Span | undefined, where Span was the span class from @opentelemetry/sdk-trace-base. This is not actually the same as you get from tracer.startActiveSpan(), and lead to some incompatibilities.

Instead, we now simply pass the proper span through, to keep this aligned. If the span is not sampled, you'll get a NonRecordingSpan (which still has all the write APIs but as noops). This also means users do not need to guard against span being undefined, which should simplify user code.

mydea · 2023-10-13T07:51:14Z

packages/opentelemetry/src/utils/contextData.ts

+ * Try to get the Propagation Context from the given OTEL context.
+ * This requires the SentryPropagator to be registered.
+ */
+export function getPropagationContextFromContext(context: Context): PropagationContext | undefined {


I expose these as functions instead of exposing the constants, which should encapsulate this better.

mydea · 2023-10-13T07:53:25Z

packages/node-experimental/package.json

-    "@prisma/instrumentation": "~5.3.1",
+    "@opentelemetry/core": "~1.17.1",
+    "@opentelemetry/context-async-hooks": "~1.17.1",
+    "@opentelemetry/instrumentation": "0.44.0",


I hard-pinned all instrumentation dependencies, and pinned the "core" dependencies to bugfixes. LMK if you think we should hard pin everything 🤔

Yeah this is fine - we can always adjust it afterwards too if it ends up breaking something.

lforst

lgtm

lforst · 2023-10-16T13:34:49Z

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

@@ -161,7 +158,7 @@ export class Http implements Integration {
      return;
    }

-    const data = getRequestSpanData(span);
+    const data = _INTERNAL.getRequestSpanData(span);


m: This _INTERNAL thing seems very smelly to me. Can we not hoist this into utils or smth?

I think it's fine because it's dependent on us making some decisions around schema/naming. Internal is sufficiently scary enough that things shouldn't be used with it, and it will be going away very soon.

yeah so this should definitely go away, I just want to avoid exposing things publicly that we need right now because then we can't remove them easily later :) once the semantic attributes stuff is merged and agreed upon this can/should just be public API!

lforst · 2023-10-16T13:53:44Z

.craft.yml

@@ -24,6 +24,10 @@ targets:
  - name: npm
    id: '@sentry/replay'
    includeNames: /^sentry-replay-\d.*\.tgz$/
+  ## 1.6. OpenTelemetry package
+  - name: npm
+    id: '@sentry/opentelemetry'


We prob also need to add this to the verdaccio config for the e2e tests.

should this not automatically be picked up there? 🤔 based on the namespace...?

No unfortunately not:

sentry-javascript/packages/e2e-tests/verdaccio-config/config.yaml

Lines 29 to 39 in cadeefe

# To not use a proxy (e.g. npm) but instead use verdaccio for package hosting we need to define rules here without the

# `proxy` field. Sadly we can't use a wildcard like "@sentry/*" because we have some dependencies (@sentry/cli,

# @sentry/webpack-plugin) that fall under that wildcard but don't live in this repository. If we were to use that

# wildcard, we would get a 404 when attempting to install them, since they weren't uploaded to verdaccio, and also

# don't have a proxy configuration.

'@sentry/angular':

access: $all

publish: $all

unpublish: $all

# proxy: npmjs # Don't proxy for E2E tests!

damn, good catch! I'll actually add this here as well, is missing there so far: https://github.com/getsentry/sentry-javascript/blob/develop/docs/new-sdk-release-checklist.md

AbhiPrasad · 2023-10-17T00:50:46Z

packages/node-experimental/package.json

-    "@prisma/instrumentation": "~5.3.1",
+    "@opentelemetry/core": "~1.17.1",
+    "@opentelemetry/context-async-hooks": "~1.17.1",
+    "@opentelemetry/instrumentation": "0.44.0",


Yeah this is fine - we can always adjust it afterwards too if it ends up breaking something.

AbhiPrasad · 2023-10-17T00:52:54Z

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

@@ -161,7 +158,7 @@ export class Http implements Integration {
      return;
    }

-    const data = getRequestSpanData(span);
+    const data = _INTERNAL.getRequestSpanData(span);


I think it's fine because it's dependent on us making some decisions around schema/naming. Internal is sufficiently scary enough that things shouldn't be used with it, and it will be going away very soon.

AbhiPrasad · 2023-10-17T00:53:49Z

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

@@ -30,7 +30,7 @@ if (NODE_VERSION.major && NODE_VERSION.major >= 16) {
 */
 export function init(options: NodeExperimentalOptions | undefined = {}): void {
  // Ensure we register our own global hub before something else does
-  // This will register the NodeExperimentalHub as the global hub
+  // This will register the OpenTelemetryHub as the global hub
  getCurrentHub();


wait does it? Don't we have to setup an async context strategy first?

actually no, this was kind of a footgun I found while working on this - the first time getCurrentHub is called anywhere it will setup a global hub (the async context strategy also uses the global hub as a fallback), and since a bunch of the general init public APIs call getCurrentHub I had to call this here to ensure the global hub is actually our custom otel hub and not the regular one 😬

actually, what may work (??) is setting up the otel async context strategy first 🤔 not sure if that may run into problems with otel not being setup (yet) but I'll give it a try!

OK, so some more testing: this sadly doesn't fix the issue, we still get a different global hub (from core). BUT I still refactored this so that setOpenTelemetryContextAsyncContextStrategy just calls getCurrentHub under the hood, so this is hidden away from the user and they don't need to manually call getCurrentHub which is a bit weird.

Even more testing: This has weird side effects, so instead I actually export a new function setupGlobalHub to be called, to make it clearer what it does.

AbhiPrasad · 2023-10-17T00:56:42Z

packages/opentelemetry/README.md

+1. Add `SentrySpanProcessor` as span processor
+1. Add a context manager wrapped via `wrapContextManagerClass`
+1. Add `SentryPropagator` as propagator
+1. Setup OTEL-powered async context strategy for Sentry via `setOpenTelemetryContextAsyncContextStrategy()`


l: think we need to fix numbers here

this is a somewhat "weird" markdown trick that you can generate a numbered list with all 1. which will be shown as a regular, correctly numbered list, but making it much easier to reorder the list or add items in between 😅

AbhiPrasad · 2023-10-17T00:57:01Z

packages/opentelemetry/package.json

+  "author": "Sentry",
+  "license": "MIT",
+  "engines": {
+    "node": ">=8"


l: Do we have to bump this?

actually yes, we should! will do that,

AbhiPrasad · 2023-10-17T00:58:35Z

packages/opentelemetry/src/custom/client.ts

+    /** Get the OTEL tracer. */
+    public get tracer(): Tracer {
+      if (this._tracer) {
+        return this._tracer;


will we ever expose users setting their own tracer?

not right now - really there is no need to do that, they can just use any tracer directly as well separate from this. This is just a nice helper to make this a bit easier.

AbhiPrasad · 2023-10-17T00:59:02Z

packages/opentelemetry/src/custom/client.ts

+
+      // Remove `captureContext` hint and instead clone already here
+      if (hint && hint.captureContext) {
+        actualScope = OpenTelemetryScope.clone(scope);


we should just restructure all of the process event code in v8, I dislike we have to do this here.

yes, agreed!

packages/opentelemetry/src/spanProcessor.ts

packages/opentelemetry/src/types.ts

Apply suggestions from code review Co-authored-by: Abhijeet Prasad <[email protected]> PR feedback PR feedback: Expose proper `setupGlobalHub` method pr feedback & new release docs

github-actions · 2023-10-18T10:05:43Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	84 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	31.6 KB (+0.09% 🔺)
@sentry/browser - Webpack (gzipped)	21.77 KB (+0.1% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	80.5 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	29.54 KB (+0.09% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	21.71 KB (+0.1% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	245.65 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	85.63 KB (+0.13% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	61.55 KB (+0.17% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	32.66 KB (+0.07% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	84.07 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped)	21.81 KB (+0.1% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	102.21 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	49.67 KB (+0.05% 🔺)

Lms24

LGTM!

mydea requested review from lforst, Lms24 and AbhiPrasad October 13, 2023 07:41

mydea self-assigned this Oct 13, 2023

mydea commented Oct 13, 2023

View reviewed changes

mydea force-pushed the fn/opentelemetry branch 2 times, most recently from d6bf541 to bd3fa18 Compare October 13, 2023 07:49

mydea commented Oct 13, 2023

View reviewed changes

mydea force-pushed the fn/opentelemetry branch 2 times, most recently from 98dcd62 to 9bfa893 Compare October 13, 2023 14:18

lforst approved these changes Oct 16, 2023

View reviewed changes

AbhiPrasad approved these changes Oct 17, 2023

View reviewed changes

feat(opentelemetry): Add new @sentry/opentelemetry package

68bf926

Apply suggestions from code review Co-authored-by: Abhijeet Prasad <[email protected]> PR feedback PR feedback: Expose proper `setupGlobalHub` method pr feedback & new release docs

mydea force-pushed the fn/opentelemetry branch from e7c725c to 68bf926 Compare October 18, 2023 09:48

Lms24 approved these changes Oct 18, 2023

View reviewed changes

mydea merged commit dc68c09 into develop Oct 19, 2023

mydea deleted the fn/opentelemetry branch October 19, 2023 07:26

	# To not use a proxy (e.g. npm) but instead use verdaccio for package hosting we need to define rules here without the
	# `proxy` field. Sadly we can't use a wildcard like "@sentry/*" because we have some dependencies (@sentry/cli,
	# @sentry/webpack-plugin) that fall under that wildcard but don't live in this repository. If we were to use that
	# wildcard, we would get a 404 when attempting to install them, since they weren't uploaded to verdaccio, and also
	# don't have a proxy configuration.

	'@sentry/angular':
	access: $all
	publish: $all
	unpublish: $all
	# proxy: npmjs # Don't proxy for E2E tests!

Uh oh!

feat(opentelemetry): Add new @sentry/opentelemetry package #9238

feat(opentelemetry): Add new @sentry/opentelemetry package #9238

Uh oh!

Conversation

mydea commented Oct 13, 2023

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!

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!

lforst 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!

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!

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!

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!

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!

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!

github-actions bot commented Oct 18, 2023

size-limit report 📦

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat(opentelemetry): Add new `@sentry/opentelemetry` package #9238

feat(opentelemetry): Add new `@sentry/opentelemetry` package #9238