From 6d528aafa12b101288b3bb1c41d55e5df681b1ae Mon Sep 17 00:00:00 2001 From: dvoytenko Date: Wed, 16 Aug 2023 11:24:09 -0700 Subject: [PATCH 1/4] Add attributes argument to recordException API --- api/src/trace/NonRecordingSpan.ts | 6 ++- api/src/trace/span.ts | 6 ++- .../opentelemetry-sdk-trace-base/src/Span.ts | 17 ++++++-- .../test/common/Span.test.ts | 40 +++++++++++++++++++ 4 files changed, 64 insertions(+), 5 deletions(-) diff --git a/api/src/trace/NonRecordingSpan.ts b/api/src/trace/NonRecordingSpan.ts index a9e5bcaf9b..dc7e3de3d7 100644 --- a/api/src/trace/NonRecordingSpan.ts +++ b/api/src/trace/NonRecordingSpan.ts @@ -71,5 +71,9 @@ export class NonRecordingSpan implements Span { } // By default does nothing - recordException(_exception: Exception, _time?: TimeInput): void {} + recordException( + _exception: Exception, + _attributesOrStartTime?: SpanAttributes | TimeInput, + _time?: TimeInput + ): void {} } diff --git a/api/src/trace/span.ts b/api/src/trace/span.ts index d80b8c2626..061c89cf4b 100644 --- a/api/src/trace/span.ts +++ b/api/src/trace/span.ts @@ -125,5 +125,9 @@ export interface Span { * @param [time] the time to set as Span's event time. If not provided, * use the current time. */ - recordException(exception: Exception, time?: TimeInput): void; + recordException( + exception: Exception, + attributesOrStartTime?: SpanAttributes | TimeInput, + time?: TimeInput + ): void; } diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index 31fb1555ac..2d267bbb09 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -272,8 +272,19 @@ export class Span implements APISpan, ReadableSpan { return this._ended === false; } - recordException(exception: Exception, time?: TimeInput): void { - const attributes: SpanAttributes = {}; + recordException( + exception: Exception, + attributesOrStartTime?: SpanAttributes | TimeInput, + timeStamp?: TimeInput + ): void { + if (isTimeInput(attributesOrStartTime)) { + if (!isTimeInput(timeStamp)) { + timeStamp = attributesOrStartTime; + } + attributesOrStartTime = undefined; + } + + const attributes = sanitizeAttributes(attributesOrStartTime); if (typeof exception === 'string') { attributes[SemanticAttributes.EXCEPTION_MESSAGE] = exception; } else if (exception) { @@ -296,7 +307,7 @@ export class Span implements APISpan, ReadableSpan { attributes[SemanticAttributes.EXCEPTION_TYPE] || attributes[SemanticAttributes.EXCEPTION_MESSAGE] ) { - this.addEvent(ExceptionEventName, attributes, time); + this.addEvent(ExceptionEventName, attributes, timeStamp); } else { diag.warn(`Failed to record an exception ${exception}`); } diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index 11a94ffc7c..0567af8ba5 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1203,6 +1203,46 @@ describe('Span', () => { const event = span.events[0]; assert.deepStrictEqual(event.time, [0, 123]); }); + + it('should record an exception with provided time as a 3rd arg', () => { + const span = new Span( + tracer, + ROOT_CONTEXT, + name, + spanContext, + SpanKind.CLIENT + ); + // @ts-expect-error writing readonly property. performance time origin is mocked to return ms value of [1,1] + span['_performanceOffset'] = 0; + assert.strictEqual(span.events.length, 0); + span.recordException('boom', undefined, [0, 123]); + const event = span.events[0]; + assert.deepStrictEqual(event.time, [0, 123]); + }); + }); + + describe('when attributes are provided', () => { + it('should sanitized and merge attributes when provided', () => { + const span = new Span( + tracer, + ROOT_CONTEXT, + name, + spanContext, + SpanKind.CLIENT + ); + // @ts-expect-error writing readonly property. performance time origin is mocked to return ms value of [1,1] + span['_performanceOffset'] = 0; + assert.strictEqual(span.events.length, 0); + span.recordException('boom', { + ...validAttributes, + ...invalidAttributes, + } as unknown as SpanAttributes); + const event = span.events[0]; + assert.deepStrictEqual(event.attributes, { + [SemanticAttributes.EXCEPTION_MESSAGE]: 'boom', + ...validAttributes, + }); + }); }); describe('when exception code is numeric', () => { From 02fc8904315793492cc57b28bdfb664b5e0c4023 Mon Sep 17 00:00:00 2001 From: dvoytenko Date: Sun, 20 Aug 2023 14:15:38 -0700 Subject: [PATCH 2/4] make sure the passed attributes override exception and tests --- .../opentelemetry-sdk-trace-base/src/Span.ts | 5 ++- .../test/common/Span.test.ts | 33 ++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index 2d267bbb09..ea3526b29b 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -284,7 +284,7 @@ export class Span implements APISpan, ReadableSpan { attributesOrStartTime = undefined; } - const attributes = sanitizeAttributes(attributesOrStartTime); + const attributes: SpanAttributes = {}; if (typeof exception === 'string') { attributes[SemanticAttributes.EXCEPTION_MESSAGE] = exception; } else if (exception) { @@ -301,6 +301,9 @@ export class Span implements APISpan, ReadableSpan { attributes[SemanticAttributes.EXCEPTION_STACKTRACE] = exception.stack; } } + if (attributesOrStartTime) { + Object.assign(attributes, sanitizeAttributes(attributesOrStartTime)); + } // these are minimum requirements from spec if ( diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index 0567af8ba5..873865a0dc 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1233,16 +1233,47 @@ describe('Span', () => { // @ts-expect-error writing readonly property. performance time origin is mocked to return ms value of [1,1] span['_performanceOffset'] = 0; assert.strictEqual(span.events.length, 0); - span.recordException('boom', { + const exception = { code: 'Error', message: 'boom', stack: 'bar' }; + span.recordException(exception, { ...validAttributes, ...invalidAttributes, } as unknown as SpanAttributes); const event = span.events[0]; assert.deepStrictEqual(event.attributes, { + [SemanticAttributes.EXCEPTION_TYPE]: 'Error', [SemanticAttributes.EXCEPTION_MESSAGE]: 'boom', + [SemanticAttributes.EXCEPTION_STACKTRACE]: 'bar', ...validAttributes, }); }); + + it('should prioritize the provided attributes over generated', () => { + const span = new Span( + tracer, + ROOT_CONTEXT, + name, + spanContext, + SpanKind.CLIENT + ); + // @ts-expect-error writing readonly property. performance time origin is mocked to return ms value of [1,1] + span['_performanceOffset'] = 0; + assert.strictEqual(span.events.length, 0); + const exception = { code: 'Error', message: 'boom', stack: 'bar' }; + span.recordException(exception, { + [SemanticAttributes.EXCEPTION_TYPE]: 'OverrideError', + [SemanticAttributes.EXCEPTION_MESSAGE]: 'override-boom', + [SemanticAttributes.EXCEPTION_STACKTRACE]: 'override-bar', + ...validAttributes, + ...invalidAttributes, + } as unknown as SpanAttributes); + const event = span.events[0]; + assert.deepStrictEqual(event.attributes, { + ...validAttributes, + [SemanticAttributes.EXCEPTION_TYPE]: 'OverrideError', + [SemanticAttributes.EXCEPTION_MESSAGE]: 'override-boom', + [SemanticAttributes.EXCEPTION_STACKTRACE]: 'override-bar', + }); + }); }); describe('when exception code is numeric', () => { From f70a9df8198f42ac6d8dacdb81ccfac5ca1b37fd Mon Sep 17 00:00:00 2001 From: dvoytenko Date: Mon, 21 Aug 2023 11:40:30 -0700 Subject: [PATCH 3/4] update changelog --- CHANGELOG.md | 1 + api/CHANGELOG.md | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72e7508827..47fa5aee06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :rocket: (Enhancement) +* feat(api): add attributes argument to recordException API [#4071](https://github.com/open-telemetry/opentelemetry-js/pull/4071) * feat(sdk-metrics): implement MetricProducer specification [#4007](https://github.com/open-telemetry/opentelemetry-js/pull/4007) ### :bug: (Bug Fix) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 5b52050074..a1dcf67d4f 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. ## Unreleased +### :rocket: (Enhancement) + +* feat(api): add attributes argument to recordException API [#4071](https://github.com/open-telemetry/opentelemetry-js/pull/4071) + ## 1.4.1 ### :bug: (Bug Fix) From 8f71f2c88a51c585bf59f9f022c4ffbe42a04c60 Mon Sep 17 00:00:00 2001 From: dvoytenko Date: Mon, 28 Aug 2023 10:39:38 -0700 Subject: [PATCH 4/4] Reduce ambiguity in the API --- api/src/trace/span.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/api/src/trace/span.ts b/api/src/trace/span.ts index 061c89cf4b..59eb233a85 100644 --- a/api/src/trace/span.ts +++ b/api/src/trace/span.ts @@ -125,9 +125,18 @@ export interface Span { * @param [time] the time to set as Span's event time. If not provided, * use the current time. */ + recordException(exception: Exception, time?: TimeInput): void; + + /** + * Sets exception as a span event + * @param exception the exception the only accepted values are string or Error + * @param [attributes] the attributes that will be added to the error event. + * @param [time] the time to set as Span's event time. If not provided, + * use the current time. + */ recordException( exception: Exception, - attributesOrStartTime?: SpanAttributes | TimeInput, + attributes?: SpanAttributes, time?: TimeInput ): void; }