Refactor otel_sampler to use callbacks for setup, should_sample, and decription #261
Conversation
Codecov Report
@@ Coverage Diff @@
## main #261 +/- ##
==========================================
+ Coverage 36.39% 36.52% +0.13%
==========================================
Files 37 41 +4
Lines 3168 3170 +2
==========================================
+ Hits 1153 1158 +5
+ Misses 2015 2012 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I have implemented your comments, have a look at the added/changed docs and let me know what you think. |
| @@ -8,7 +8,7 @@ | |||
| stdlib, | |||
| opentelemetry_api | |||
| ]}, | |||
| {env, [{sampler, {parent_based, #{root => {always_on, #{}}}}}, % default sampler | |||
| {env, [{sampler, {otel_sampler_parent_based, #{root => {otel_sampler_always_on, #{}}}}}, % default sampler | |||
There was a problem hiding this comment.
Oh, now I see the use of the modules being named like always_on.
Hm... We definitely can't have the module names be stuff like always_on but maybe the configuration should special case the builtin samplers instead of requiring the full module name?
I'm not sure it is worth it if it risks confusion and it is "write once" basically so not a big deal... @bryannaegele do you have any thoughts on something like this?
There was a problem hiding this comment.
I can see it both ways. One reason to keep it atoms would be to keep the naming consistent across languages. It would probably be unnecessarily confusing to SREs to have to configure a whole module name in the env var vs using the same setting across all services.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think it would be worthwhile to do a survey of the other SDKs and see if there is variance already. If it's all over the place, then we don't need to constrain ourselves.
There was a problem hiding this comment.
Agh, that shows me that now there is "jaeger_remote", never even heard o that... and some added propagators.
|
Looks like there are some elixir test failures now @dvic |
Fixed :)! |
| should_sample(Ctx, _TraceId, _Links, _SpanName, _SpanKind, _Attributes, _Opts) -> | ||
| {?DROP, [], tracestate(Ctx)}. | ||
|
|
||
| tracestate(Ctx) -> |
There was a problem hiding this comment.
Sorry, was about to merge this but giving it one last look over and noticed this function is duplicated in every sampler. I think it can just use otel_span:tracestate(Ctx) in the call above instead of having its own function for this in the sampler.
My guess as to why this was the case before is simply because the span_ctx record defaults to undefined for the tracestate field. This will have to be changed to default to [] and I can't think of any harm in that.
I guess a tiny put of extra memory will be used per-span_ctx...
There was a problem hiding this comment.
Sure, but shouldn't it be
SpanCtx = otel_tracer:current_span_ctx(Ctx),
otel_span:tracestate(SpanCtx)The next problem is that otel_tracer:current_span_ctx can return undefined and I assume Erlang does not have a || operator 🙂?
There was a problem hiding this comment.
Yes. And yea, should update the otel_span function to match on undefined (important to never crash so functions that might get called with an undefined by a user need to work.
Question is... should it return [] or undefined for tracestate when called without a span context... I want to say [], but could see an argument for undefined.
But I'd say just go with returning [] unless someone has a good argument against that :).
So in otel_span:
tracestate(#span_ctx{tracestate=Tracestate}) ->
Tracestate;
tracestate(_) ->
[].
_ instead of undefined just to be extra cautious about not crashing.
There was a problem hiding this comment.
I just pushed a commit. It currently now also checks tracestate(#span_ctx{tracestate=undefined}) -> because if I update span_ctx to have [] as default for the tracestate field, other tests start to fail. Is this also an acceptable solution?
There was a problem hiding this comment.
Yea, thats good, thanks.
I'll check on the tests that fail if that is changed after we get this merged.
This PR refactors the
otel_samplerto a fully fledged behavior that introduces callbackssetup/1,should_sample/7, anddescription/1for theotel_samplerbehavior. Furthermore, the built-in samplers now support passing in decision result attributes.