opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

Add WithStatus option for use with RecordError

Open treuherz opened this issue 3 years ago • 3 comments

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.

treuherz avatar May 05 '22 10:05 treuherz

CLA Signed

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 is 33.3%.

Impacted file tree graph

@@           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:

codecov[bot] avatar May 05 '22 10:05 codecov[bot]

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?

treuherz avatar May 09 '22 10:05 treuherz