From f72fbb39915cd3acd480f1b427577da2bd1cb3f6 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Wed, 30 Aug 2023 07:04:34 -0700 Subject: [PATCH 1/2] feat(api): add attributes argument to recordException API Originally introduced in #4071 but reverted in #4137 Co-authored-by: Marc Pichler --- CHANGELOG.md | 1 + api/CHANGELOG.md | 2 + api/src/trace/NonRecordingSpan.ts | 6 +- api/src/trace/span.ts | 13 +++ .../opentelemetry-sdk-trace-base/src/Span.ts | 18 ++++- .../test/common/Span.test.ts | 80 +++++++++++++++++++ 6 files changed, 117 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fd7fb4232b..16238cba8db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -344,6 +344,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se ### :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) * feat: update PeriodicExportingMetricReader and PrometheusExporter to accept optional metric producers [#4077](https://github.com/open-telemetry/opentelemetry-js/pull/4077) @aabmass diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 758829935e1..0c4b15700be 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -9,6 +9,8 @@ All notable changes to this project will be documented in this file. ### :rocket: (Enhancement) +* feat(api): add attributes argument to recordException API [#5333](https://github.com/open-telemetry/opentelemetry-js/pull/5333) @brianphillips + ### :bug: (Bug Fix) ### :books: (Refine Doc) diff --git a/api/src/trace/NonRecordingSpan.ts b/api/src/trace/NonRecordingSpan.ts index 9ee3d28837a..4d0467daa51 100644 --- a/api/src/trace/NonRecordingSpan.ts +++ b/api/src/trace/NonRecordingSpan.ts @@ -80,5 +80,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 27abe854298..2abf8e3cef8 100644 --- a/api/src/trace/span.ts +++ b/api/src/trace/span.ts @@ -149,4 +149,17 @@ export interface Span { * 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, + attributes?: SpanAttributes, + time?: TimeInput + ): void; } diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index f28f74a3e1e..8d9205fd42c 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -318,7 +318,18 @@ export class SpanImpl implements Span { return this._ended === false; } - recordException(exception: Exception, time?: TimeInput): void { + recordException( + exception: Exception, + attributesOrStartTime?: Attributes | TimeInput, + timeStamp?: TimeInput + ): void { + if (isTimeInput(attributesOrStartTime)) { + if (!isTimeInput(timeStamp)) { + timeStamp = attributesOrStartTime; + } + attributesOrStartTime = undefined; + } + const attributes: Attributes = {}; if (typeof exception === 'string') { attributes[SEMATTRS_EXCEPTION_MESSAGE] = exception; @@ -335,13 +346,16 @@ export class SpanImpl implements Span { attributes[SEMATTRS_EXCEPTION_STACKTRACE] = exception.stack; } } + if (attributesOrStartTime) { + Object.assign(attributes, sanitizeAttributes(attributesOrStartTime)); + } // these are minimum requirements from spec if ( attributes[SEMATTRS_EXCEPTION_TYPE] || attributes[SEMATTRS_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 cff451c43c2..4b22664cf69 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1438,6 +1438,86 @@ 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 SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + name, + spanContext, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + // @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 SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + name, + spanContext, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + // @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, { + ...validAttributes, + ...invalidAttributes, + } as unknown as Attributes); + const event = span.events[0]; + assert.deepStrictEqual(event.attributes, { + [SEMATTRS_EXCEPTION_TYPE]: 'Error', + [SEMATTRS_EXCEPTION_MESSAGE]: 'boom', + [SEMATTRS_EXCEPTION_STACKTRACE]: 'bar', + ...validAttributes, + }); + }); + + it('should prioritize the provided attributes over generated', () => { + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + name, + spanContext, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + // @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, { + [SEMATTRS_EXCEPTION_TYPE]: 'OverrideError', + [SEMATTRS_EXCEPTION_MESSAGE]: 'override-boom', + [SEMATTRS_EXCEPTION_STACKTRACE]: 'override-bar', + ...validAttributes, + ...invalidAttributes, + } as unknown as Attributes); + const event = span.events[0]; + assert.deepStrictEqual(event.attributes, { + ...validAttributes, + [SEMATTRS_EXCEPTION_TYPE]: 'OverrideError', + [SEMATTRS_EXCEPTION_MESSAGE]: 'override-boom', + [SEMATTRS_EXCEPTION_STACKTRACE]: 'override-bar', + }); + }); }); describe('when exception code is numeric', () => { From 66558e915ec9315d602c9f7cf0f83e51623854e7 Mon Sep 17 00:00:00 2001 From: Brian Phillips <28457+brianphillips@users.noreply.github.com> Date: Wed, 22 Jan 2025 19:53:51 -0600 Subject: [PATCH 2/2] fix(api): clean up method signature and logic flow This clarifies the `recordException` method signature, cleans up the logic sequence and avoids an extra invocation of `sanitizeAttributes(...)`. See [this comment](https://github.com/open-telemetry/opentelemetry-js/pull/5333/files#r1926210496) for context Co-authored-by: Godfrey Chan --- api/src/trace/NonRecordingSpan.ts | 13 +++++++--- api/src/trace/span.ts | 6 ++++- .../opentelemetry-sdk-trace-base/src/Span.ts | 26 ++++++++++--------- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/api/src/trace/NonRecordingSpan.ts b/api/src/trace/NonRecordingSpan.ts index 4d0467daa51..572d3bfbb48 100644 --- a/api/src/trace/NonRecordingSpan.ts +++ b/api/src/trace/NonRecordingSpan.ts @@ -81,8 +81,13 @@ export class NonRecordingSpan implements Span { // By default does nothing recordException( - _exception: Exception, - _attributesOrStartTime?: SpanAttributes | TimeInput, - _time?: TimeInput - ): void {} + exception: Exception, + time?: TimeInput + ): void; + recordException( + exception: Exception, + attributes: SpanAttributes | undefined, + time?: TimeInput + ): void; + recordException(): void {} } diff --git a/api/src/trace/span.ts b/api/src/trace/span.ts index 2abf8e3cef8..a749c7929ae 100644 --- a/api/src/trace/span.ts +++ b/api/src/trace/span.ts @@ -159,7 +159,11 @@ export interface Span { */ recordException( exception: Exception, - attributes?: SpanAttributes, + time?: TimeInput + ): void; + recordException( + exception: Exception, + attributes: SpanAttributes | undefined, time?: TimeInput ): void; } diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index 8d9205fd42c..1db4d1f9a45 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -318,18 +318,17 @@ export class SpanImpl implements Span { return this._ended === false; } + recordException(exception: Exception, time?: TimeInput): void; recordException( exception: Exception, - attributesOrStartTime?: Attributes | TimeInput, - timeStamp?: TimeInput + attributes: Attributes | undefined, + time?: TimeInput + ): void; + recordException( + exception: Exception, + attributesOrTime?: Attributes | TimeInput, + time?: TimeInput ): void { - if (isTimeInput(attributesOrStartTime)) { - if (!isTimeInput(timeStamp)) { - timeStamp = attributesOrStartTime; - } - attributesOrStartTime = undefined; - } - const attributes: Attributes = {}; if (typeof exception === 'string') { attributes[SEMATTRS_EXCEPTION_MESSAGE] = exception; @@ -346,8 +345,11 @@ export class SpanImpl implements Span { attributes[SEMATTRS_EXCEPTION_STACKTRACE] = exception.stack; } } - if (attributesOrStartTime) { - Object.assign(attributes, sanitizeAttributes(attributesOrStartTime)); + + if (isTimeInput(attributesOrTime)) { + time = attributesOrTime; + } else { + Object.assign(attributes, sanitizeAttributes(attributesOrTime)); } // these are minimum requirements from spec @@ -355,7 +357,7 @@ export class SpanImpl implements Span { attributes[SEMATTRS_EXCEPTION_TYPE] || attributes[SEMATTRS_EXCEPTION_MESSAGE] ) { - this.addEvent(ExceptionEventName, attributes, timeStamp); + this.addEvent(ExceptionEventName, attributes, time); } else { diag.warn(`Failed to record an exception ${exception}`); }