MEDIUM: otel: fast path for non-recording (sampled-out) streams - #8
Conversation
When the sampler decides not to record a trace, the root span is a
non-recording span and everything attached to it is discarded by the SDK.
The filter nevertheless kept doing all of its per-scope work for the rest
of the stream: evaluating attribute/event/baggage/link/status samples,
creating and finishing every child span, recording exceptions. Only the
export was saved, so a sampled-out stream cost almost as much as a fully
recorded one: with a 9-span scope set (HAProxy 3.4.2, h2load -c 64,
nbthread 4, loopback origin) 'trace_id_ratio_based' ratio 0.0 -- nothing
exported at all -- still cut the maximum throughput from 108k to 25k req/s.
'rate-limit' bounds the filter's cost on the first hop, but it does not
cover the sampler path: a stream it leaves out emits no 'traceparent' at
all, and every downstream HAProxy that extracts a parent context with
sampled=0 goes through the non-recording path whatever its own rate-limit
is.
The C wrapper already exposes 'is_recording' for this purpose, so use it:
- the runtime context remembers whether the root span is recording
('flag_norec') and keeps a pointer to the root span;
- flt_otel_scope_run_span() sets the flag right after a root span has
been created and turns out not to be recording;
- flt_otel_scope_run() then skips the spans that do not inject a
context and evaluates no attribute, event, baggage, link or status
sample for the spans it still creates; exceptions are skipped as well;
- flt_otel_scope_span_init() resolves a skipped parent to the root span,
so that the injecting span is still created (non-recording, sampled
flag cleared) and 'traceparent' keeps reaching the downstream
services, which therefore follow the sampling decision.
With the same benchmark ratio 0.0 goes from 25.1k to 52.3k req/s (2.08x)
and ratio 0.1 from 22.4k to 38.2k (1.7x); ratio 1.0 is unchanged as the
fast path is never taken. The remaining cost is the single non-recording
root span itself (about 7 us per request, mostly in the C wrapper), which
cannot be avoided as long as the sampling decision is made inside
StartSpan.
'finish' of a skipped span only triggers the existing debug warning.
'rate-limit', 'otel-stop' and 'require-context' are unaffected.
8f0cc70 to
37ddbcd
Compare
|
Hello @ygkat, Thanks for taking the time to open this PR and address this issue! I reviewed the approach and decided to implement it a bit differently, so I’ve added my own commit to address the same problem. I really appreciate you taking the initiative to look into this and put together a solution. Even though I went with a different implementation, your PR helped highlight the issue and get it on my radar. I’ve also pushed my alternative implementation to the pr-8 branch b3a8ae5 if you’d like to take a look at it. Feel free to compare the approaches and let me know what you think! Thanks again for the contribution and for taking the time to improve the project! |
|
Hi @zaga00, Thanks for the rework. You are right on both points I had missed: baggage must be evaluated regardless of the sampling decision, and a per-stream decision is wrong when a I benchmarked
I see why you kept creation unconditional: the configuration model assumes every declared span exists ( The direction I have in mind, on top of
Does that fit how you want the filter to behave, or should the "every declared span exists" rule stay absolute? I would rather follow your intent here than push a design you do not want to maintain. |
|
Hello @ygkat, Thanks for checking with me before you started working on this. The rule does not have to stay absolute, but I want it bent behind an explicit instrumentation option, default off, and with a narrower notion of a skippable span than "does not inject". Today a finish, a link, a set-var-ctx or a log-record that names a span which does not exist is already a no-op (a debug build warns). Only 'parent' is strict: a missing parent is a runtime error. So the parent fallback is the real semantic change, and that is the part I want out of the default. A span can only be skipped when nothing reads it: no inject, no baggage (a child copies the parent's baggage at creation, so a skipped span with baggage disappears from the descendants' headers), and no set-var-ctx or The fallback has to go through the scope-span entry and be re-validated when it is used, never through a cached span handle: the ancestor may have been finished before the child is created, which today is an error, and a stale handle is the use-after-free of the first patch again. Two more things: the condition on the sampler is only "local_not_sampled = always_off"; the remote delegates never apply to a child of a span we created, and the same guarantee holds for always_off and for trace_id_ratio_based. And the wrapper has no call that tells the filter which sampler a tracer runs, so "known at startup" needs a new wrapper API first; until then the option is the admin's statement that the sampler qualifies. Overall, I would rather keep the rule that every declared span exists. The saving is limited to not-sampled streams under one sampler regime, and a second class of missing span with its own fallback rules is more semantics than I want to maintain. |
|
Hi @zaga00, Thanks for the thorough explanation, that all makes sense. The stale root handle is a fair catch, and I agree the extra semantics are not worth it for a saving limited to one sampler regime. Let's keep the rule that every declared span exists. Please go ahead with |
When the sampler leaves a trace out, the SDK discards the attributes, the events, the links, the status and the exceptions of every span of that trace, so the filter evaluated sample expressions whose results were thrown away. Each span is now created before its samples are evaluated and asked whether it records; the ones that do not skip the evaluation entirely. The decision is taken per span and kept on the runtime scope span, because a sampler may record the child of a span it left out; the 'parent_based' sampler does so when its 'local_not_sampled' or 'remote_not_sampled' delegate says 'always_on'. Everything that the downstream services rely on is left in place: each configured span is created, the injected 'traceparent' carries the trace with the sampled flag cleared, and the baggage samples are evaluated whatever the sampling decision was. The span creation moved out of the span execution into a function of its own, and so did the evaluation of the attribute, event and status samples. The bundled wrapper stand-in received the 'is_recording' operation it was missing, the samplers section of README-configuration now describes what a trace that is not sampled still produces, and the implementation review follows the new order of the scope run. This reworked the patch proposed by Yeonggi Kim, with thanks for the original implementation and for the measurements that showed what a stream the sampler leaves out costs: #8
|
Hello @ygkat, the commit was added to the main branch, it differs from the one in the pr-8 branch by the added documentation. Thank you for your contribution to the project and your helpful comments. Also, refinements with record-off spans are possible to further speed up the work, but that’s it for now. |
2 participants
Summary
When the sampler decides not to record a trace, the root span is a non-recording span and everything attached to it is discarded by the SDK. The filter nevertheless keeps doing all of its per-scope work for the rest of the stream: it evaluates attribute/event/baggage/link/status samples, creates and finishes every child span and records exceptions. Only the export is saved, so a sampled-out stream costs almost as much as a fully recorded one.
rate-limitis the documented way to bound the filter's cost, and it works well for the first hop. It does not cover the sampler path, though: a stream left out byrate-limitemits notraceparentat all (downstream services start their own traces), and every downstream HAProxy that extracts a parent context withsampled=0goes through the non-recording path regardless of its ownrate-limit. This PR makes that path cheap by usingis_recording, which the C wrapper already exposes for exactly this purpose.Measurements
HAProxy 3.4.2, filter v2.2.0, wrapper v3.3.0, SDK 1.28.0,
nbthread 4,h2load --h1 -c 64 -n 100000, loopback origin (http-request return), 9-span scope set with header injection.Changes
include/scope.h:flt_otel_runtime_contextgainsflag_norecandroot_span.src/event.cflt_otel_scope_run_span(): after a root span is created, queryis_recording; if it is not recording, setflag_norecand remember the root.flt_otel_scope_run(): withflag_norecset, spans that do not inject a context are skipped entirely, and no attribute/event/baggage/link/status sample is evaluated for the spans that are still created; exceptions are skipped as well.src/scope.cflt_otel_scope_span_init(): withflag_norecset, a parent that was skipped resolves to the root span instead of failing.Context propagation is preserved: injecting spans are still created (non-recording, sampled flag cleared), so
traceparentkeeps reaching downstream services and they follow the sampling decision.finishof a skipped span only triggers the existing debug warning.rate-limit,otel-stopandrequire-contextare unaffected.Verification
http-after-response otel-groupscopes), downstream receivestraceparentwithsampled=0, the collector receives no spans.Remaining cost and possible follow-ups
A filter that is attached but creates no span runs at the no-filter baseline (101k), while a single non-recording root span costs about 7 µs per request (60.7k). That root span is unavoidable as long as the sampling decision is made inside
StartSpan;perfattributes most of its cost to the C wrapper (span-handle table lookups,shared_ptrcopies of Tracer/Span/Context, amake_shared<Context>per StartSpan) rather than to the SDK. Two follow-ups would close the remaining gap, both out of scope here: