opentelemetry-go
opentelemetry-go copied to clipboard
Add WithStatus option for use with RecordError
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 to RecordError
either all need to be EventOption, which I've achieved by embedding
EventOption in ErrorOption. This results in needing to implement a
no-op applyEvent for our one "pure" ErrorOption, and a no-op
applyError on the existing EventOptions. The only alternative I
can see would be doing the embedding the other way around, but then I
think RecordError would need to filter through its options to figure
out which ones to forward to addEvent. Happy to take a steer from the
maintainers here.
This is made a bit more confusing by WithStackTrace. It's EventOption
but it only has any effect if passed to RecordError, not AddEvent
(see #2336). I think this should be changed to an ErrorOption as it's
currently easy to misuse. This would simplify RecordError a bit as we
wouldn't have to process the event options twice in order to catch the
stacktrace, but the existence of EventConfig.StackTrace() makes that
tricky to do while maintaining perfect backwards-compatibility.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: treuherz / name: Eli Treuherz (201ed00b5b948a39a65f7c0a34f7da6e612df4ae)
Codecov Report
Merging #2887 (201ed00) into main (c7cf945) will decrease coverage by
0.1%. The diff coverage is33.3%.
@@ 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
| Impacted Files | Coverage Δ | |
|---|---|---|
| bridge/opentracing/internal/mock.go | 63.8% <0.0%> (-3.3%) |
:arrow_down: |
| internal/global/trace.go | 90.7% <0.0%> (+1.6%) |
:arrow_up: |
| trace/noop.go | 70.0% <0.0%> (+3.3%) |
:arrow_up: |
| trace/trace.go | 98.2% <ø> (ø) |
|
| trace/config.go | 54.8% <14.2%> (-9.4%) |
:arrow_down: |
| sdk/trace/span.go | 88.3% <92.8%> (+0.4%) |
:arrow_up: |
| exporters/jaeger/jaeger.go | 90.3% <0.0%> (-0.9%) |
:arrow_down: |
| sdk/trace/batch_span_processor.go | 82.1% <0.0%> (+1.8%) |
:arrow_up: |
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 WithStackTrace issue I mentioned above?