Add WithStatus option for use with RecordError #2887
Conversation
treuherz
requested review from
jmacd,
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
paivagustavo,
MadVikingGod,
pellared and
hanyuancheung
as code owners
|
|
Fixes open-telemetry#1677 by adding a functional option that sets the Status of a span when calling RecordError.
treuherz
force-pushed
the
record-error-with-status
branch
from
7b605f0
to
201ed00
Compare
Codecov Report
@@ Coverage Diff @@
## main #2887 +/- ##
=======================================
- Coverage 75.7% 75.6% -0.2%
=======================================
Files 177 177
Lines 11819 11850 +31
=======================================
+ Hits 8950 8960 +10
- Misses 2636 2657 +21
Partials 233 233
|
| @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Add a `WithStatus` option to `go.opentelemetry.io/otel/trace` to allow setting the span status when calling `span.RecordError`. (#1677) | |||
There was a problem hiding this comment.
I think saying to the trace package should be enough here.
There was a problem hiding this comment.
Please keep the full package name go.opentelemetry.io/otel/trace. It addresses the ambiguity of which trace package is being discussed and provides new users who are searching the docs a universality recognized package name.
| @@ -357,7 +357,7 @@ type Span interface { | |||
| // additional call to SetStatus is required if the Status of the Span should | |||
| // be set to Error, as this method does not change the Span status. If this | |||
| // span is not being recorded or err is nil then this method does nothing. | |||
| RecordError(err error, options ...EventOption) | |||
| RecordError(err error, options ...ErrorOption) | |||
There was a problem hiding this comment.
I think changing this may warrant a major release.
There was a problem hiding this comment.
I think there must be an argument covariance/contravariance case I've missed here. I'll see if I can find a way of doing it and keeping full backwards compat.
| setErrorStatus bool | ||
| } | ||
|
|
||
| func (cfg *ErrorConfig) SetErrorStatus() bool { |
There was a problem hiding this comment.
Calling this SetErrorStatus kind of hints at a setter, which this isn't. How about naming the variable hasErrorStatus, and this method HasErrorStatus?
| @@ -171,17 +171,14 @@ func (nonRecordingSpan) IsRecording() bool { return false } | |||
| // SetStatus does nothing. | |||
| func (nonRecordingSpan) SetStatus(codes.Code, string) {} | |||
|
|
|||
| // SetError does nothing. | |||
| func (nonRecordingSpan) SetError(bool) {} | |||
There was a problem hiding this comment.
I'm not sure why this method is being removed? It may be a cleanup. But then, I think it'd be better to do the removal in its own PR.
There was a problem hiding this comment.
Ah, sorry about that. I removed it as it looked like it was defunct, then decided I shouldn't be doing things like that in the same PR but didn't clean up all my clean-ups!
There was a problem hiding this comment.
We cannot accept changes to the public API of this package. For more information please refer to our versioning policy. Please revert all changes that introduce compatibility issues.
|
I understand, I'll have another stab at doing it backwards-compatibly. I don't know if you're planning through to the next major release yet, but is it worth me submitting a separate MR that's deliberately not backwards-compatible and trying to clean up both this and the |
Fixes #1677 by adding a functional option that sets the Status of a
span when calling RecordError.
I'm not completely sure about the way I've tweaked the interfaces here.
So that they can be passed to
addEvent, the options toRecordErroreither all need to be
EventOption, which I've achieved by embeddingEventOptioninErrorOption. This results in needing to implement ano-op
applyEventfor our one "pure"ErrorOption, and a no-opapplyErroron the existingEventOptions. The only alternative Ican see would be doing the embedding the other way around, but then I
think
RecordErrorwould need to filter through its options to figureout which ones to forward to
addEvent. Happy to take a steer from themaintainers here.
This is made a bit more confusing by
WithStackTrace. It'sEventOptionbut it only has any effect if passed to
RecordError, notAddEvent(see #2336). I think this should be changed to an
ErrorOptionas it'scurrently easy to misuse. This would simplify
RecordErrora bit as wewouldn't have to process the event options twice in order to catch the
stacktrace, but the existence of
EventConfig.StackTrace()makes thattricky to do while maintaining perfect backwards-compatibility.