feat: allow setting attributes when recording exceptions #97
Conversation
vreynolds
commented
Jun 23, 2021
vreynolds
commented
- span.recordException needs to allow settings event attributes per spec
- Fixes Span.recordException must allow additional attributes according to spec #95
vreynolds
requested review from
dyladan,
Flarna,
johnbley,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 94.84% 94.69% -0.16%
==========================================
Files 42 42
Lines 563 565 +2
Branches 87 87
==========================================
+ Hits 534 535 +1
- Misses 29 30 +1
Continue to review full report at Codecov.
|
| * @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( |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
I like that. For symmetry, I would change the addEvent as well, since they're pretty much identical interfaces.
|
Generally I like the change. Before approving and merging I'm trying to get guidance from the TC on the backport policy. Since this will require us to go to 1.1.x, will we need to backport fixes to 1.0.x in the future. |
Seems backporting fixes will be required for the duration of the 1.x lifecycle, and it is not recommended to make changes if possible. I think this change is not high enough priority to break this recommendation. A user can simply call |
| * @param [startTime] start time of the event. | ||
| */ | ||
| addEvent( | ||
| name: string, | ||
| attributesOrStartTime?: SpanAttributes | TimeInput, | ||
| attributes?: SpanAttributes, |
There was a problem hiding this comment.
| attributes?: SpanAttributes, | |
| attributes: SpanAttributes, |
No need to make this optional here. The other overload allows already a call with name only.
| */ | ||
| recordException( | ||
| exception: Exception, | ||
| attributes?: SpanAttributes, |
There was a problem hiding this comment.
| attributes?: SpanAttributes, | |
| attributes: SpanAttributes, |
| * @param [startTime] start time of the event. | ||
| */ | ||
| addEvent( | ||
| name: string, | ||
| attributesOrStartTime?: SpanAttributes | TimeInput, | ||
| attributes?: SpanAttributes, |
There was a problem hiding this comment.
strictly speaking this is a breaking change for a typescript user as addEvent(name, timeInput, timeInput) is no longer allowed now.
There was a problem hiding this comment.
Ha, that's an interseting edge case! Closing this PR either way, but thanks for catching.
@dyladan I didn't see an |
|
It was just a typo. I meant Re-reading the issue and my response though has made me realize that the feature request was to add attributes to the event, not the span. |
|
I definitely would like this to be reconsidered! I have multiple use cases that need this. |
|
@dyladan I’m not familiar with process of getting certain work in particular releases or who is needed to get alignment. Any next steps I can help with? I didn’t want to miss this going in next minor release. Let me know. |
|
There is no exact process. At the last couple SIG meetings we had decided to release 1.1 when #147, #142, and #129 were merged which has now happened. Now we're preparing the SDK so that the release can be as close to the same time as possible. If you want to make an argument that 1.1 should be held for this feature, I would suggest you make a feature request issue (or a PR I guess) and tag the JS maintainers team. If it is not in 1.1, it will have to wait for 1.2 |
