From 4f7c300581f483a59b79b2fb0f1a23cc5835d6ab Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 22 Jan 2025 18:00:46 -0800 Subject: [PATCH] refactor(api)!: Use TypeScript overload signature for `addEvent` This makes the intent more clear, and makes the invalid signature `addEvent(name, timeStamp1, timeStamp2)` explicitly illegal in TS. This has always been the intent, and the textual documentation somewhat implies that already. Technically, this is breaking change for TypeScript users. There are situations previously valid TS code will stop being accepted by the type checker, even if the code is semantically valid. It mostly arises when the caller is trying to propagate the same-ish overload signature. It can still be done safely, just requires a bit more care. In any case, this will go into the 2.0 release so it's a great time to make this change. --- api/src/trace/NonRecordingSpan.ts | 8 ++++++- api/src/trace/span.ts | 8 +++---- .../opentelemetry-sdk-trace-base/src/Span.ts | 24 +++++++++++-------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/api/src/trace/NonRecordingSpan.ts b/api/src/trace/NonRecordingSpan.ts index 9ee3d28837a..4c48c5ae0c0 100644 --- a/api/src/trace/NonRecordingSpan.ts +++ b/api/src/trace/NonRecordingSpan.ts @@ -49,7 +49,13 @@ export class NonRecordingSpan implements Span { } // By default does nothing - addEvent(_name: string, _attributes?: SpanAttributes): this { + addEvent(name: string, startTime?: TimeInput): this; + addEvent( + name: string, + attributes: SpanAttributes | undefined, + startTime?: TimeInput + ): this; + addEvent(): this { return this; } diff --git a/api/src/trace/span.ts b/api/src/trace/span.ts index 27abe854298..243ea182827 100644 --- a/api/src/trace/span.ts +++ b/api/src/trace/span.ts @@ -68,14 +68,14 @@ export interface Span { * Adds an event to the Span. * * @param name the name of the event. - * @param [attributesOrStartTime] the attributes that will be added; these are - * associated with this event. Can be also a start time - * if type is {@type TimeInput} and 3rd param is undefined + * @param [attributes] the attributes that will be added; these are + * associated with this event. * @param [startTime] start time of the event. */ + addEvent(name: string, startTime?: TimeInput): this; addEvent( name: string, - attributesOrStartTime?: SpanAttributes | TimeInput, + attributes: SpanAttributes | undefined, startTime?: TimeInput ): this; diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index f28f74a3e1e..39e477d0a9e 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -179,13 +179,18 @@ export class SpanImpl implements Span { /** * * @param name Span Name - * @param [attributesOrStartTime] Span attributes or start time - * if type is {@type TimeInput} and 3rd param is undefined + * @param [attributes] Span attributes for the event * @param [timeStamp] Specified time stamp for the event */ + addEvent(name: string, timeStamp?: TimeInput): this; addEvent( name: string, - attributesOrStartTime?: Attributes | TimeInput, + attributes: Attributes | undefined, + timeStamp?: TimeInput + ): this; + addEvent( + name: string, + attributesOrTimestamp?: Attributes | TimeInput, timeStamp?: TimeInput ): this { if (this._isSpanEnded()) return this; @@ -202,14 +207,13 @@ export class SpanImpl implements Span { this._droppedEventsCount++; } - if (isTimeInput(attributesOrStartTime)) { - if (!isTimeInput(timeStamp)) { - timeStamp = attributesOrStartTime; - } - attributesOrStartTime = undefined; - } + let attributes: Attributes | undefined = undefined; - const attributes = sanitizeAttributes(attributesOrStartTime); + if (isTimeInput(attributesOrTimestamp)) { + timeStamp = attributesOrTimestamp; + } else { + attributes = sanitizeAttributes(attributesOrTimestamp); + } this.events.push({ name,