Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

feat: allow setting attributes when recording exceptions #97

Closed
Closed
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
12 changes: 10 additions & 2 deletions src/trace/NonRecordingSpan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ export class NonRecordingSpan implements Span {
}

// By default does nothing
addEvent(_name: string, _attributes?: SpanAttributes): this {
addEvent(
_name: string,
_attributesOrTime?: SpanAttributes | TimeInput,
_time?: TimeInput
): this {
return this;
}

Expand All @@ -71,5 +75,9 @@ export class NonRecordingSpan implements Span {
}

// By default does nothing
recordException(_exception: Exception, _time?: TimeInput): void {}
recordException(
_exception: Exception,
_attributesOrTime?: SpanAttributes | TimeInput,
_time?: TimeInput
): void {}
}
32 changes: 27 additions & 5 deletions src/trace/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,21 @@ 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 [startTime] start time of the event.
*/
addEvent(name: string, startTime?: TimeInput): this;

/**
* Adds an event to the Span.
*
* @param name the name of the event.
* @param [attributes] the attributes that will be added; these are
* associated with this event.
* @param [startTime] start time of the event.
*/
addEvent(
name: string,
attributesOrStartTime?: SpanAttributes | TimeInput,
attributes?: SpanAttributes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attributes?: SpanAttributes,
attributes: SpanAttributes,

No need to make this optional here. The other overload allows already a call with name only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strictly speaking this is a breaking change for a typescript user as addEvent(name, timeInput, timeInput) is no longer allowed now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, that's an interseting edge case! Closing this PR either way, but thanks for catching.

startTime?: TimeInput
): this;

Expand Down Expand Up @@ -120,10 +127,25 @@ export interface Span {
isRecording(): boolean;

/**
* Sets exception as a span event
* Sets exception as a span event.
*
* @param exception the exception the only accepted values are string or Error
* @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] additional attributes to be associated with this event.
* @param [time] the time to set as Span's event time. If not provided,
* use the current time.
*/
recordException(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use an overload here to be more specific? e.g. like this:

recordException(exception: Exception, time?: TimeInput): void;
recordException(exception: Exception, attributes: SpanAttributes, time?: TimeInput): void;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that. For symmetry, I would change the addEvent as well, since they're pretty much identical interfaces.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in new commit

exception: Exception,
attributes?: SpanAttributes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attributes?: SpanAttributes,
attributes: SpanAttributes,

time?: TimeInput
): void;
}