Unify the Config type across instrumentation #226
Conversation
Codecov Report
@@ Coverage Diff @@
## main #226 +/- ##
==========================================
- Coverage 76.93% 76.63% -0.31%
==========================================
Files 41 43 +2
Lines 2012 2037 +25
==========================================
+ Hits 1548 1561 +13
- Misses 422 433 +11
- Partials 42 43 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The config is mostly copied from signalfx#226
Added a use-case in the |
| ctx context.Context | ||
| tracer trace.Tracer | ||
| defaultStartOpts []trace.SpanStartOption | ||
| *internal.Config |
There was a problem hiding this comment.
any reason why not embedding as value? it should avoid some memory allocations
There was a problem hiding this comment.
The returned value from NewConfig is a pointer.
If we want to optimize I would want to build out benchmarks first. Definitely something we can do in the future if needed.
| copy(merged[len(c.defaultStartOpts):], opts) | ||
| } | ||
| return merged | ||
| type optConv struct { |
There was a problem hiding this comment.
how about renaming to optionConv to have it similar to optionFunc?
| trace.WithAttributes(semconv.DBOperationKey.String("Iterator")), | ||
| ) | ||
| _, span := c.resolveTracer().Start(c.ctx, "Iterator", sso...) | ||
| _, span := c.ResolveTracer(c.ctx).Start(c.ctx, "Iterator", sso...) |
There was a problem hiding this comment.
I find it awkward to have the the ctx in the config struct.
I propose to have two methods in the common config:
- ResolveTracer
- ResolveTracerContext (when some call have context)
Same for WithSpan.
Then if the instrumented library used context we use ResolveTracerContext and WithSpanContext (e.g. for http) if not then we simply use ResolveTracerContext and WithSpan (e.g. for leveldb).
Calling trace.SpanFromContext(ctx) is useless if we know that it will never return anything.
If you disagree I would rather put context.Background() in all places instead of cfg.ctx. Current code can give a false impression that can be some real context.
There was a problem hiding this comment.
I find it awkward to have the the
ctxin the config struct.
Sure, agreed. However, this is an artifact of the underlying library not accepting a context as an argument. The instrumentation consequentially needs to accept this and apply it as needed.
I propose to have two methods in the common config:
- ResolveTracer
- ResolveTracerContext (when some call have context)
ResolveTracer is equivalent to just Config.Tracer. If you don't have a context to extract a TracerProvider from it should not be used.
Same for
WithSpan.
The context passed to WithSpan is updated to include the span and passed to the closure. Having the ability to set the parent is going to be important for more than just span propagation. Like the rest of the Go ecosystem, having a context as a first argument is a good idea in that it keeps context propagation explicit and adding *Context type functions are useful only in that they allow backwards compatibility for context propagation. Having one from the start seems to be the opposite of idiomatic Go.
If you disagree I would rather put
context.Background()in all places instead ofcfg.ctx. Current code can give a false impression that can be some real context.
That can in fact be a real context, with a span embedded in it. Passing that context as included in this PR ensures the correct user specified TracerProvider is used and heredity is preserved.
* Initial instrumentation layout * Add changes to changelog * Add initial config and instrumentation The config is mostly copied from #226 * Implement instrumentation Remove unneeded Config methods. * Add integration tests * Shutdown TracerProvider in tests * Update Middleware docs * Fix example test
The Config and its related Options are recreated for all our instrumentation currently. This results in a lot of duplicate code and tests. This adds a common Config type with appropriate functionality as well as common options. These internal types and functions are expected to be reused in instrumentation libraries by embedding the types or forwarding calls to functions.