Otelmux span options #5251
Otelmux span options #5251
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5251 +/- ##
=======================================
- Coverage 61.3% 61.2% -0.1%
=======================================
Files 186 185 -1
Lines 11253 11218 -35
=======================================
- Hits 6907 6876 -31
+ Misses 4147 4142 -5
- Partials 199 200 +1
|
|
@dmathieu @hanyuancheung Any updates on merging this PR? Since it's approved for 2 weeks |
|
Only @open-telemetry/go-maintainers can merge PRs. |
There was a problem hiding this comment.
I am not sure if it is good to allow users to do that much.
The user can e.g. change the span kind using https://pkg.go.dev/go.opentelemetry.io/otel/trace#WithSpanKind or create attributes which are not aligned with the semantic conventions and they others may blame that the instrumentation library does not follow semantic conventions.
so folks can (for example), enable stack traces to record errors.
I am not sure if this is a good justification. This instrumentation library is not recording any errors.
I think we can discuss during the SIG meeting if we should make instrumentation libraries so configurable.
I am aware that there is a precedent that otelhttp and otelgrpc already have WithSpanOptions but maybe we should retrospect this.
| @@ -86,6 +88,22 @@ func WithSpanNameFormatter(fn func(routeName string, r *http.Request) string) Op | |||
| }) | |||
| } | |||
|
|
|||
| // WithSpanStartOption applies options to all the HTTP span created by the | |||
| // instrumentation. | |||
| func WithSpanStartOption(o ...oteltrace.SpanStartOption) Option { | |||
There was a problem hiding this comment.
Absolutely. I'll leave the code as is for now, since this may not be relevant, if it is replaced with the option to add stacktraces instead - see this comment.
I'll return to this, if we decide to keep the function.
|
@pellared Thank you for the review.
This sounds like a very valid point to me. In #5250 you suggest adding the option to capture stacktraces instead. Does that apply here as well? |
No as this instrumentation library does not handle panics and does not call https://pkg.go.dev/go.opentelemetry.io/otel/trace#Span.RecordError. Anyway, I am really grateful for your positive attitude 👍 |
Ah, I see. That makes sense. |
This allows setting span start and end options to the otelmux configuration, so folks can (for example), enable stack traces to record errors.
Heavily inspired by #5250.
Since
otelmuxdiffers a bit from theginimplementation, I'm not sure if adapting thetracewareto includeSpanStartOptionsandSpanEndOptionsis the right way to go. If not, please let me know how it should be adapted.Closes #5139.