ref(node-otel): Avoid exporting internals & refactor attribute adding (#8920)

mydea · web-flow · commit 9666506f8938 · 2023-09-01T09:49:03.000+02:00
I noticed that we kind of extended the public API by exporting some
things from opentelemetry-node that we need in node-experimental. I am
deprecating these internals as I'm refactoring some things to be a bit
cleaner IMHO.

The one export we actually need outside I am exporting as
`_INTERNAL_getSentrySpan` to make it clear this is not meant to be used
otherwise. The main reason I am not just exporting this is that this
_may_ have to change if we change how we internally store/handle spans -
e.g. if we move away from the map this is just not possible to have
anymore, and I do not want to lock us into having to support this in the
future.
diff --git a/packages/node-experimental/src/integrations/express.ts b/packages/node-experimental/src/integrations/express.ts
@@ -1,8 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
 import type { Integration } from '@sentry/types';
 
+import { addOriginToOtelSpan } from '../utils/addOriginToSpan';
 import { NodePerformanceIntegration } from './NodePerformanceIntegration';
 
 /**
@@ -31,9 +31,7 @@ export class Express extends NodePerformanceIntegration<void> implements Integra
     return [
       new ExpressInstrumentation({
         requestHook(span) {
-          addOtelSpanData(span.spanContext().spanId, {
-            origin: 'auto.http.otel.express',
-          });
+          addOriginToOtelSpan(span, 'auto.http.otel.express');
         },
       }),
     ];
diff --git a/packages/node-experimental/src/integrations/fastify.ts b/packages/node-experimental/src/integrations/fastify.ts
@@ -1,8 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { FastifyInstrumentation } from '@opentelemetry/instrumentation-fastify';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
 import type { Integration } from '@sentry/types';
 
+import { addOriginToOtelSpan } from '../utils/addOriginToSpan';
 import { NodePerformanceIntegration } from './NodePerformanceIntegration';
 
 /**
@@ -31,9 +31,7 @@ export class Fastify extends NodePerformanceIntegration<void> implements Integra
     return [
       new FastifyInstrumentation({
         requestHook(span) {
-          addOtelSpanData(span.spanContext().spanId, {
-            origin: 'auto.http.otel.fastify',
-          });
+          addOriginToOtelSpan(span, 'auto.http.otel.fastify');
         },
       }),
     ];
diff --git a/packages/node-experimental/src/integrations/graphql.ts b/packages/node-experimental/src/integrations/graphql.ts
@@ -1,8 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { GraphQLInstrumentation } from '@opentelemetry/instrumentation-graphql';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
 import type { Integration } from '@sentry/types';
 
+import { addOriginToOtelSpan } from '../utils/addOriginToSpan';
 import { NodePerformanceIntegration } from './NodePerformanceIntegration';
 
 /**
@@ -32,9 +32,7 @@ export class GraphQL extends NodePerformanceIntegration<void> implements Integra
       new GraphQLInstrumentation({
         ignoreTrivialResolveSpans: true,
         responseHook(span) {
-          addOtelSpanData(span.spanContext().spanId, {
-            origin: 'auto.graphql.otel-graphql',
-          });
+          addOriginToOtelSpan(span, 'auto.graphql.otel.graphql');
         },
       }),
     ];
diff --git a/packages/node-experimental/src/integrations/http.ts b/packages/node-experimental/src/integrations/http.ts
@@ -4,10 +4,9 @@ import { registerInstrumentations } from '@opentelemetry/instrumentation';
 import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
 import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-node';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
-import { hasTracingEnabled } from '@sentry/core';
+import { hasTracingEnabled, Transaction } from '@sentry/core';
 import { getCurrentHub } from '@sentry/node';
-import type { AdditionalOtelSpanData } from '@sentry/opentelemetry-node';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
+import { _INTERNAL_getSentrySpan } from '@sentry/opentelemetry-node';
 import type { EventProcessor, Hub, Integration } from '@sentry/types';
 import type { ClientRequest, IncomingMessage, ServerResponse } from 'http';
 
@@ -146,34 +145,37 @@ export class Http implements Integration {
     const data = getRequestSpanData(span, request, response);
     const { attributes } = span;
 
-    const additionalData: AdditionalOtelSpanData = {
-      tags: {},
-      data: {
+    const sentrySpan = _INTERNAL_getSentrySpan(span.spanContext().spanId);
+    if (sentrySpan) {
+      sentrySpan.origin = 'auto.http.otel.http';
+
+      const additionalData: Record<string, unknown> = {
         url: data.url,
-      },
-      contexts: {},
-      metadata: {},
-      origin: 'auto.http.otel.http',
-    };
-
-    if (span.kind === SpanKind.SERVER) {
-      additionalData.metadata = { request };
-    }
+      };
 
-    if (attributes[SemanticAttributes.HTTP_STATUS_CODE]) {
-      const statusCode = attributes[SemanticAttributes.HTTP_STATUS_CODE] as string;
-      additionalData.tags['http.status_code'] = statusCode;
-      additionalData.data['http.response.status_code'] = statusCode;
-    }
+      if (sentrySpan instanceof Transaction && span.kind === SpanKind.SERVER) {
+        sentrySpan.setMetadata({ request });
+      }
 
-    if (data['http.query']) {
-      additionalData.data['http.query'] = data['http.query'].slice(1);
-    }
-    if (data['http.fragment']) {
-      additionalData.data['http.fragment'] = data['http.fragment'].slice(1);
-    }
+      if (attributes[SemanticAttributes.HTTP_STATUS_CODE]) {
+        const statusCode = attributes[SemanticAttributes.HTTP_STATUS_CODE] as string;
+        additionalData['http.response.status_code'] = statusCode;
 
-    addOtelSpanData(span.spanContext().spanId, additionalData);
+        sentrySpan.setTag('http.status_code', statusCode);
+      }
+
+      if (data['http.query']) {
+        additionalData['http.query'] = data['http.query'].slice(1);
+      }
+      if (data['http.fragment']) {
+        additionalData['http.fragment'] = data['http.fragment'].slice(1);
+      }
+
+      Object.keys(additionalData).forEach(prop => {
+        const value = additionalData[prop];
+        sentrySpan.setData(prop, value);
+      });
+    }
 
     if (this._breadcrumbs) {
       getCurrentHub().addBreadcrumb(
diff --git a/packages/node-experimental/src/integrations/mongo.ts b/packages/node-experimental/src/integrations/mongo.ts
@@ -1,8 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { MongoDBInstrumentation } from '@opentelemetry/instrumentation-mongodb';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
 import type { Integration } from '@sentry/types';
 
+import { addOriginToOtelSpan } from '../utils/addOriginToSpan';
 import { NodePerformanceIntegration } from './NodePerformanceIntegration';
 
 /**
@@ -31,9 +31,7 @@ export class Mongo extends NodePerformanceIntegration<void> implements Integrati
     return [
       new MongoDBInstrumentation({
         responseHook(span) {
-          addOtelSpanData(span.spanContext().spanId, {
-            origin: 'auto.db.otel-mongo',
-          });
+          addOriginToOtelSpan(span, 'auto.db.otel.mongo');
         },
       }),
     ];
diff --git a/packages/node-experimental/src/integrations/mongoose.ts b/packages/node-experimental/src/integrations/mongoose.ts
@@ -1,8 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { MongooseInstrumentation } from '@opentelemetry/instrumentation-mongoose';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
 import type { Integration } from '@sentry/types';
 
+import { addOriginToOtelSpan } from '../utils/addOriginToSpan';
 import { NodePerformanceIntegration } from './NodePerformanceIntegration';
 
 /**
@@ -31,9 +31,7 @@ export class Mongoose extends NodePerformanceIntegration<void> implements Integr
     return [
       new MongooseInstrumentation({
         responseHook(span) {
-          addOtelSpanData(span.spanContext().spanId, {
-            origin: 'auto.db.otel-mongoose',
-          });
+          addOriginToOtelSpan(span, 'auto.db.otel.mongoose');
         },
       }),
     ];
diff --git a/packages/node-experimental/src/integrations/mysql2.ts b/packages/node-experimental/src/integrations/mysql2.ts
@@ -1,8 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { MySQL2Instrumentation } from '@opentelemetry/instrumentation-mysql2';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
 import type { Integration } from '@sentry/types';
 
+import { addOriginToOtelSpan } from '../utils/addOriginToSpan';
 import { NodePerformanceIntegration } from './NodePerformanceIntegration';
 
 /**
@@ -31,9 +31,7 @@ export class Mysql2 extends NodePerformanceIntegration<void> implements Integrat
     return [
       new MySQL2Instrumentation({
         responseHook(span) {
-          addOtelSpanData(span.spanContext().spanId, {
-            origin: 'auto.db.otel-mysql2',
-          });
+          addOriginToOtelSpan(span, 'auto.db.otel.mysql2');
         },
       }),
     ];
diff --git a/packages/node-experimental/src/integrations/postgres.ts b/packages/node-experimental/src/integrations/postgres.ts
@@ -1,8 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { PgInstrumentation } from '@opentelemetry/instrumentation-pg';
-import { addOtelSpanData } from '@sentry/opentelemetry-node';
 import type { Integration } from '@sentry/types';
 
+import { addOriginToOtelSpan } from '../utils/addOriginToSpan';
 import { NodePerformanceIntegration } from './NodePerformanceIntegration';
 
 /**
@@ -31,9 +31,7 @@ export class Postgres extends NodePerformanceIntegration<void> implements Integr
     return [
       new PgInstrumentation({
         requestHook(span) {
-          addOtelSpanData(span.spanContext().spanId, {
-            origin: 'auto.db.otel-postgres',
-          });
+          addOriginToOtelSpan(span, 'auto.db.otel.postgres');
         },
       }),
     ];
diff --git a/packages/node-experimental/src/utils/addOriginToSpan.ts b/packages/node-experimental/src/utils/addOriginToSpan.ts
@@ -0,0 +1,13 @@
+import type { Span as OtelSpan } from '@opentelemetry/api';
+import { _INTERNAL_getSentrySpan } from '@sentry/opentelemetry-node';
+import type { SpanOrigin } from '@sentry/types';
+
+/** Adds an origin to an OTEL Span. */
+export function addOriginToOtelSpan(otelSpan: OtelSpan, origin: SpanOrigin): void {
+  const sentrySpan = _INTERNAL_getSentrySpan(otelSpan.spanContext().spanId);
+  if (!sentrySpan) {
+    return;
+  }
+
+  sentrySpan.origin = origin;
+}
diff --git a/packages/opentelemetry-node/src/index.ts b/packages/opentelemetry-node/src/index.ts
@@ -1,3 +1,19 @@
+import { getSentrySpan } from './spanprocessor';
+
 export { SentrySpanProcessor } from './spanprocessor';
 export { SentryPropagator } from './propagator';
-export * from './utils/spanData';
+
+/* eslint-disable deprecation/deprecation */
+export { addOtelSpanData, getOtelSpanData, clearOtelSpanData } from './utils/spanData';
+export type { AdditionalOtelSpanData } from './utils/spanData';
+/* eslint-enable deprecation/deprecation */
+
+/**
+ * This is only exported for internal use.
+ * Semver etc. does not apply here, this is subject to change at any time!
+ * This is explicitly _NOT_ public because we may have to change the underlying way we store/handle spans,
+ * which may make this API unusable without further notice.
+ *
+ * @private
+ */
+export { getSentrySpan as _INTERNAL_getSentrySpan };
diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts
@@ -10,19 +10,19 @@ import { SENTRY_DYNAMIC_SAMPLING_CONTEXT_KEY, SENTRY_TRACE_PARENT_CONTEXT_KEY }
 import { isSentryRequestSpan } from './utils/isSentryRequest';
 import { mapOtelStatus } from './utils/mapOtelStatus';
 import { parseSpanDescription } from './utils/parseOtelSpanDescription';
-import { clearOtelSpanData, getOtelSpanData } from './utils/spanData';
 
-export const SENTRY_SPAN_PROCESSOR_MAP: Map<SentrySpan['spanId'], SentrySpan> = new Map<
-  SentrySpan['spanId'],
-  SentrySpan
->();
+export const SENTRY_SPAN_PROCESSOR_MAP: Map<string, SentrySpan> = new Map<string, SentrySpan>();
 
 // make sure to remove references in maps, to ensure this can be GCed
 function clearSpan(otelSpanId: string): void {
-  clearOtelSpanData(otelSpanId);
   SENTRY_SPAN_PROCESSOR_MAP.delete(otelSpanId);
 }
 
+/** Get a Sentry span for an otel span ID. */
+export function getSentrySpan(otelSpanId: string): SentrySpan | undefined {
+  return SENTRY_SPAN_PROCESSOR_MAP.get(otelSpanId);
+}
+
 /**
  * Converts OpenTelemetry Spans to Sentry Spans and sends them to Sentry via
  * the Sentry SDK.
@@ -225,18 +225,10 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi
 
   const { op, description, data } = parseSpanDescription(otelSpan);
 
-  const { data: additionalData, tags, origin } = getOtelSpanData(otelSpan.spanContext().spanId);
-
   sentrySpan.setStatus(mapOtelStatus(otelSpan));
   sentrySpan.setData('otel.kind', SpanKind[kind]);
 
-  if (tags) {
-    Object.keys(tags).forEach(prop => {
-      sentrySpan.setTag(prop, tags[prop]);
-    });
-  }
-
-  const allData = { ...attributes, ...data, ...additionalData };
+  const allData = { ...attributes, ...data };
 
   Object.keys(allData).forEach(prop => {
     const value = allData[prop];
@@ -245,52 +237,27 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi
 
   sentrySpan.op = op;
   sentrySpan.description = description;
-
-  if (origin) {
-    sentrySpan.origin = origin;
-  }
 }
 
 function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelSpan): void {
   const { op, description, source, data } = parseSpanDescription(otelSpan);
-  const { data: additionalData, tags, contexts, metadata, origin } = getOtelSpanData(otelSpan.spanContext().spanId);
 
   transaction.setContext('otel', {
     attributes: otelSpan.attributes,
     resource: otelSpan.resource.attributes,
   });
 
-  if (tags) {
-    Object.keys(tags).forEach(prop => {
-      transaction.setTag(prop, tags[prop]);
-    });
-  }
-
-  if (metadata) {
-    transaction.setMetadata(metadata);
-  }
-
-  const allData = { ...data, ...additionalData };
+  const allData = data || {};
 
   Object.keys(allData).forEach(prop => {
     const value = allData[prop];
     transaction.setData(prop, value);
   });
 
-  if (contexts) {
-    Object.keys(contexts).forEach(prop => {
-      transaction.setContext(prop, contexts[prop]);
-    });
-  }
-
   transaction.setStatus(mapOtelStatus(otelSpan));
 
   transaction.op = op;
   transaction.setName(description, source);
-
-  if (origin) {
-    transaction.origin = origin;
-  }
 }
 
 function convertOtelTimeToSeconds([seconds, nano]: [number, number]): number {
diff --git a/packages/opentelemetry-node/src/utils/spanData.ts b/packages/opentelemetry-node/src/utils/spanData.ts
