Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sdk-trace-base): avoid keeping non-string status.message on Span#setStatus() #4999

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se

### :bug: (Bug Fix)

* fix(sdk-trace-base): avoid keeping non-string `status.message` on `Span#setStatus()` [#4999](https://github.com/open-telemetry/opentelemetry-js/pull/4999) @pichlermarc

### :books: (Refine Doc)

### :house: (Internal)
Expand Down
14 changes: 13 additions & 1 deletion packages/opentelemetry-sdk-trace-base/src/Span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,19 @@ export class Span implements APISpan, ReadableSpan {

setStatus(status: SpanStatus): this {
if (this._isSpanEnded()) return this;
this.status = status;
this.status = { ...status };

// 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;
}
dyladan marked this conversation as resolved.
Show resolved Hide resolved

return this;
}

Expand Down
26 changes: 26 additions & 0 deletions packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,32 @@ describe('Span', () => {
message: 'This is an error',
});
span.end();

assert.strictEqual(span.status.code, SpanStatusCode.ERROR);
assert.strictEqual(span.status.message, 'This is an error');
});

it('should drop non-string status message', function () {
const warnStub = sinon.spy(diag, 'warn');
const span = new Span(
tracer,
ROOT_CONTEXT,
name,
spanContext,
SpanKind.CLIENT
);
span.setStatus({
code: SpanStatusCode.ERROR,
message: new Error('this is not a string') as any,
});
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 return ReadableSpan', () => {
Expand Down