-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement tail sampling on the client #4160
Conversation
@philipp-spiess at least in the Go SDK, it's possible to set the trace This is also how we implement |
The native sampling flag behaviour sounds like pretty much what you want here, but I may be missing some nuance - feel free to ping me on Slack to discuss if that might be more effective! |
@bobheadxi I'll look into it but one problem I remember is that for autocomplete we only know towards the end of the trace if we will sample it so all sub-spans that are already flushed need to wait for that, hence the tail sampling approach. If we can do this with |
@bobheadxi I tried (via
|
It should be a read-only attribute, so the should-trace flag must be set by a custom Node version: https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_sdk_trace_base.Sampler.html
Curious how this works, as your custom sampler in this PR interrogates the root span 🤔 do we set the attribute only at the end of the root span? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions on native mechanism: #4160 (comment) - if this is not critical, I recommend investigating if we can use some semblance of the native mechanisms instead, as the bookkeeping done in this exporter seems a bit risky 😁
Approving to unblock regardless as this is just telemetry and if we need this to improve the state of trace export traffic, then feel free to ship this as-is :)
@bobheadxi Yeah. I only wanted to sample autocomplete requests that also end up being shown to users. We do a similar thing for telemetry, because we don't really care about those which aren't shown (these won't be noticed by a user anyhow and we only care about their cost which we can capture from the gateway traces). So what we do here is we start a trace and then do all the fancy shmancy to build the autocomplete result and only if the request was not interrupted in the mean time (i.e by another keystroke or some other things) will it be marked as "suggested" and also sampled. That's the inherent issue I had with custom Sampler, the decision on wether something samples need to be made ahead of time. I'll try this out for now, my main motivation behind this PR is that we reduce the load of traces being sent from the Cody Clients to the OTel collectors so if we run into issues we can continue to move the sampling decision into the otel collector but it would be nice if the client doesn't log every single I/O operation going on 😬 |
Sounds good! |
Closes CODY-1367
We currently implement open telemetry tail-sampling only on the OTel Collector. This causes a bunch of issues, though:
This PR attempts to implement tail-sampling like we do it on the OTel collector on the client. Since there is no Tail Sampler, this is done by extending the HTTP SpanExporter. For every span that is exported, we will now look at the root span and only record if if it was manually marked as sampled.
Since spans can be flushed out of order (sub-spans can be flushed before the parent span is sent), theres a bit of bookkeeping that queues spans until their parent was received (or up to 1min in total).
Test plan
CodyTraceExport
line 17 forthis.isTracingEnabled
CodyTraceExport
on line 58