fix(span): enforce StatusCode precedence rules in setStatus per specification

CHANGELOG.md

@@ -23,6 +23,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2
 
 ### :bug: Bug Fixes
 
+* fix(sdk-trace-base): enforce StatusCode precedence rules in `setStatus` per specification [#6461](https://github.com/open-telemetry/opentelemetry-js/pull/6461) @newbee1939
 * fix(sdk-trace-web): propagate `optimised` flag in `getElementXPath` recursion [#6335](https://github.com/open-telemetry/opentelemetry-js/pull/6335) @akkupratap323
 
 ### :books: Documentation

api/src/trace/span.ts

@@ -89,11 +89,21 @@ export interface Span {
   addLinks(links: Link[]): this;
 
   /**
-   * Sets a status to the span. If used, this will override the default Span
-   * status. Default is {@link SpanStatusCode.UNSET}. SetStatus overrides the value
-   * of previous calls to SetStatus on the Span.
+   * Sets the status of the span.
    *
-   * @param status the SpanStatus to set.
+   * By default, a span has status {@link SpanStatusCode.UNSET}.
+   * Calling this method overrides that default.
+   *
+   * The status codes have a total order: `OK > ERROR > UNSET`.
+   *
+   * - Once {@link SpanStatusCode.OK} is set, any further attempts to change
+   *   the status are ignored.
+   * - Any attempt to set {@link SpanStatusCode.UNSET} is always ignored.
+   *
+   * The `message` field is only used when {@link SpanStatusCode.ERROR} is set.
+   * For all other status codes, `message` is ignored.
+   *
+   * @param status The {@link SpanStatus} to set.
    */
   setStatus(status: SpanStatus): this;
 

packages/opentelemetry-sdk-trace-base/src/Span.ts

@@ -241,19 +241,26 @@ export class SpanImpl implements Span {
 
   setStatus(status: SpanStatus): this {
     if (this._isSpanEnded()) return this;
-    this.status = { ...status };
+    if (status.code === SpanStatusCode.UNSET) return this;
+    if (this.status.code === SpanStatusCode.OK) return this;
+
+    const newStatus: SpanStatus = { code: status.code };
 
     // When using try-catch, the caught "error" is of type `any`. When then assigning `any` to `status.message`,
     // TypeScript will not error. While this can happen during use of any API, it is more common on Span#setStatus()
     // as it's likely used in a catch-block. Therefore, we validate if `status.message` is actually a string, null, or
     // undefined to avoid an incorrect type causing issues downstream.
-    if (this.status.message != null && typeof status.message !== 'string') {
-      diag.warn(
-        `Dropping invalid status.message of type '${typeof status.message}', expected 'string'`
-      );
-      delete this.status.message;
+    if (status.code === SpanStatusCode.ERROR) {
+      if (typeof status.message === 'string') {
+        newStatus.message = status.message;
+      } else if (status.message != null) {
+        diag.warn(
+          `Dropping invalid status.message of type '${typeof status.message}', expected 'string'`
+        );
+      }
     }
 
+    this.status = newStatus;
     return this;
   }
 

packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts

@@ -940,51 +940,248 @@ describe('Span', () => {
     assert.strictEqual(span.events.length, 0);
   });
 
-  it('should set an error status', () => {
-    const span = new SpanImpl({
-      scope: tracer.instrumentationScope,
-      resource: tracer['_resource'],
-      context: ROOT_CONTEXT,
-      spanContext,
-      name,
-      kind: SpanKind.CLIENT,
-      spanLimits: tracer.getSpanLimits(),
-      spanProcessor: tracer['_spanProcessor'],
+  describe('setStatus', () => {
+    it('should set an error status', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({
+        code: SpanStatusCode.ERROR,
+        message: 'This is an error',
+      });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(span.status.message, 'This is an error');
     });
-    span.setStatus({
-      code: SpanStatusCode.ERROR,
-      message: 'This is an error',
+
+    it('should set an OK status', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({ code: SpanStatusCode.OK });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.OK);
+      assert.strictEqual(span.status.message, undefined);
     });
-    span.end();
 
-    assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
-    assert.strictEqual(span.status.message, 'This is an error');
-  });
+    it('should ignore attempts to set UNSET from initial state', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
 
-  it('should drop non-string status message', function () {
-    const warnStub = sinon.spy(diag, 'warn');
-    const span = new SpanImpl({
-      scope: tracer.instrumentationScope,
-      resource: tracer['_resource'],
-      context: ROOT_CONTEXT,
-      spanContext,
-      name,
-      kind: SpanKind.CLIENT,
-      spanLimits: tracer.getSpanLimits(),
-      spanProcessor: tracer['_spanProcessor'],
+      span.setStatus({ code: SpanStatusCode.UNSET });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.UNSET);
     });
-    span.setStatus({
-      code: SpanStatusCode.ERROR,
-      message: new Error('this is not a string') as any,
+
+    it('should drop non-string status message', function () {
+      const warnStub = sinon.spy(diag, 'warn');
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({
+        code: SpanStatusCode.ERROR,
+        message: new Error('this is not a string') as any,
+      });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(span.status.message, undefined);
+      sinon.assert.calledOnceWithExactly(
+        warnStub,
+        "Dropping invalid status.message of type 'object', expected 'string'"
+      );
     });
-    span.end();
 
-    assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
-    assert.strictEqual(span.status.message, undefined);
-    sinon.assert.calledOnceWithExactly(
-      warnStub,
-      "Dropping invalid status.message of type 'object', expected 'string'"
-    );
+    it('should ignore message for OK status', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({ code: SpanStatusCode.OK, message: 'should be ignored' });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.OK);
+      assert.strictEqual(span.status.message, undefined);
+    });
+
+    it('should ignore message for UNSET status', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({
+        code: SpanStatusCode.UNSET,
+        message: 'should be ignored',
+      });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.UNSET);
+      assert.strictEqual(span.status.message, undefined);
+    });
+
+    it('should ignore attempts to set UNSET status', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({ code: SpanStatusCode.ERROR, message: 'error' });
+      span.setStatus({ code: SpanStatusCode.UNSET });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(span.status.message, 'error');
+    });
+
+    it('should not allow overwriting OK status with ERROR', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({ code: SpanStatusCode.OK });
+      span.setStatus({ code: SpanStatusCode.ERROR, message: 'error' });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.OK);
+      assert.strictEqual(span.status.message, undefined);
+    });
+
+    it('should not allow overwriting OK status with UNSET', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({ code: SpanStatusCode.OK });
+      span.setStatus({ code: SpanStatusCode.UNSET });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.OK);
+    });
+
+    it('should allow overwriting ERROR status with OK', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({ code: SpanStatusCode.ERROR, message: 'error' });
+      span.setStatus({ code: SpanStatusCode.OK });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.OK);
+      assert.strictEqual(span.status.message, undefined);
+    });
+
+    it('should allow overwriting ERROR with another ERROR', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({ code: SpanStatusCode.ERROR, message: 'first' });
+      span.setStatus({ code: SpanStatusCode.ERROR, message: 'second' });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(span.status.message, 'second');
+    });
+
+    it('should not update status after span is ended', () => {
+      const span = new SpanImpl({
+        scope: tracer.instrumentationScope,
+        resource: tracer['_resource'],
+        context: ROOT_CONTEXT,
+        spanContext,
+        name,
+        kind: SpanKind.CLIENT,
+        spanLimits: tracer.getSpanLimits(),
+        spanProcessor: tracer['_spanProcessor'],
+      });
+
+      span.setStatus({
+        code: SpanStatusCode.ERROR,
+        message: 'This is an error',
+      });
+      span.end();
+
+      span.setStatus({
+        code: SpanStatusCode.OK,
+        message: 'OK',
+      });
+
+      assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
+      assert.strictEqual(span.status.message, 'This is an error');
+    });
   });
 
   it('should return ReadableSpan', () => {
@@ -1179,33 +1376,6 @@ describe('Span', () => {
     assert.strictEqual(span.events.length, 2);
   });
 
-  it('should return ReadableSpan with new status', () => {
-    const span = new SpanImpl({
-      scope: tracer.instrumentationScope,
-      resource: tracer['_resource'],
-      context: ROOT_CONTEXT,
-      spanContext,
-      name,
-      kind: SpanKind.CLIENT,
-      spanLimits: tracer.getSpanLimits(),
-      spanProcessor: tracer['_spanProcessor'],
-    });
-    span.setStatus({
-      code: SpanStatusCode.ERROR,
-      message: 'This is an error',
-    });
-    assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
-    assert.strictEqual(span.status.message, 'This is an error');
-    span.end();
-
-    // shouldn't update status
-    span.setStatus({
-      code: SpanStatusCode.OK,
-      message: 'OK',
-    });
-    assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
-  });
-
   it('should only end a span once', () => {
     const span = new SpanImpl({
       scope: tracer.instrumentationScope,