fix(opentelemetry): Do not stomp span status when startSpan callbac…

…k throws (#11170)
getsentry · Mar 25, 2024 · 284e90d · 284e90d
1 parent 32b6a99
commit 284e90d
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 9 deletions.
diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts
@@ -85,12 +85,10 @@ function ensureTimestampInSeconds(timestamp: number): number {
 
 /**
  * Convert a span to a JSON representation.
- * Note that all fields returned here are optional and need to be guarded against.
- *
- * Note: Because of this, we currently have a circular type dependency (which we opted out of in package.json).
- * This is not avoidable as we need `spanToJSON` in `spanUtils.ts`, which in turn is needed by `span.ts` for backwards compatibility.
- * And `spanToJSON` needs the Span class from `span.ts` to check here.
  */
+// Note: Because of this, we currently have a circular type dependency (which we opted out of in package.json).
+// This is not avoidable as we need `spanToJSON` in `spanUtils.ts`, which in turn is needed by `span.ts` for backwards compatibility.
+// And `spanToJSON` needs the Span class from `span.ts` to check here.
 export function spanToJSON(span: Span): Partial<SpanJSON> {
   if (spanIsSentrySpan(span)) {
     return span.getSpanJSON();

diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts
@@ -178,7 +178,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): TransactionEvent {
     data: attributes,
     origin,
     op,
-    status: getStatusMessage(status),
+    status: getStatusMessage(status), // As per protocol, span status is allowed to be undefined
   });
 
   const transactionEvent: TransactionEvent = {
@@ -252,7 +252,7 @@ function createAndFinishSpanForOtelSpan(node: SpanNode, spans: SpanJSON[], remai
     start_timestamp: convertOtelTimeToSeconds(startTime),
     // This is [0,0] by default in OTEL, in which case we want to interpret this as no end time
     timestamp: convertOtelTimeToSeconds(endTime) || undefined,
-    status: getStatusMessage(status),
+    status: getStatusMessage(status), // As per protocol, span status is allowed to be undefined
     op,
     origin,
     _metrics_summary: getMetricSummaryJsonForSpan(span as unknown as Span),

diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts
@@ -12,6 +12,7 @@ import {
   getDynamicSamplingContextFromClient,
   getRootSpan,
   handleCallbackErrors,
+  spanToJSON,
 } from '@sentry/core';
 import type { Client, Scope } from '@sentry/types';
 import { continueTraceAsRemoteSpan, getSamplingDecision, makeTraceState } from './propagator';
@@ -46,7 +47,10 @@ export function startSpan<T>(options: OpenTelemetrySpanContext, callback: (span:
     return handleCallbackErrors(
       () => callback(span),
       () => {
-        span.setStatus({ code: SpanStatusCode.ERROR });
+        // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses
+        if (spanToJSON(span).status === undefined) {
+          span.setStatus({ code: SpanStatusCode.ERROR });
+        }
       },
       () => span.end(),
     );
@@ -82,7 +86,10 @@ export function startSpanManual<T>(
     return handleCallbackErrors(
       () => callback(span, () => span.end()),
       () => {
-        span.setStatus({ code: SpanStatusCode.ERROR });
+        // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses
+        if (spanToJSON(span).status === undefined) {
+          span.setStatus({ code: SpanStatusCode.ERROR });
+        }
       },
     );
   });