feat: support to inject tracerProvider #1202
Conversation
Signed-off-by: Min-Uk.Lee <[email protected]>
minuk-dev
force-pushed
the
observability-tracerprovider
branch
from
c9b228e to
6cb85f2
Compare
| // WithTracerProvider sets the tracer provider to use for creating spans. | ||
| func WithTracerProvider(tracerProvider trace.TracerProvider) OTelObservabilityServiceOption { | ||
| return func(os *OTelObservabilityService) { | ||
| if tracerProvider != nil { |
There was a problem hiding this comment.
Why not let them set it to 'nil' if they want?
There was a problem hiding this comment.
To prevent, panic.
If someone want to disable trace, we can use noop.TracerProvider.
https://github.com/open-telemetry/opentelemetry-go/blob/trace/v1.38.0/trace/noop/noop.go#L36
I think it's the same context with #1202 (comment)
| // OTelObservabilityService implements the ObservabilityService interface from cloudevents | ||
| type OTelObservabilityService struct { | ||
| traceProvider trace.TracerProvider | ||
|
|
There was a problem hiding this comment.
Can you remove this blank line? If anything I would expect 'traceProvider' and 'tracer' to be grouped and the blank line after 'tracer'. But I'm not sure we need a line break with only 4 fields
| opt(o) | ||
| } | ||
|
|
||
| o.tracer = o.traceProvider.Tracer( |
There was a problem hiding this comment.
Would it be better/safer to check o.traceProvider for 'nil' and either set tracer to nil or call o.traceProvider.Tracer ?
There was a problem hiding this comment.
I don't know oTel, but if you allow/check for nil then you can decide if 'nil' for traceProvider means 'nil' for 'tracer too, or to default traceProvider to otel.GetTracerProvider()
There was a problem hiding this comment.
I followed the below rule. I think it's very useful way to set a guard.
2 participants
WithTracerProvideras an option.