Autocomplete: Improve sampling code and prepare for Honeycomb export #3034
Conversation
philipp-spiess
changed the title
| } | ||
|
|
||
| return handleSuccess(response) | ||
| } catch (error) { | ||
| return catchError(error) | ||
| } finally { | ||
| span.end() |
There was a problem hiding this comment.
This was causing the issue we saw with traces because for async responses, span.end would be called synchronously after creating the promise but before it resolved which left all spans be very short 😬
There was a problem hiding this comment.
would be called synchronously after creating the promise but before it resolved, which left all spans be very short
TY for catching this! Makes sense.
|
I made some more changes, now we have:
This also updates the tail sampler config used but I feel more confident that this provides actual value now, check: 
With the following sampler config: processors:
tail_sampling:
decision_wait: 30s
num_traces: 100000
expected_new_traces_per_sec: 10
policies:
name: and
type: and
and:
and_sub_policy:
# Only track events that originate from the completion provider
- name: is-completion-event
type: ottl_condition
ottl_condition:
error_mode: ignore
span:
- "name == \"autocomplete.provideInlineCompletionItems\""
# Only track spans that were manually marked as to be sampled by the client
- name: is-sampled
type: ottl_condition
ottl_condition:
error_mode: ignore
span:
- "attributes[\"sampled\"] == true" |
|
Did some more cleanup now. Here's a recent example: https://ui.honeycomb.io/sourcegraph/environments/cody/datasets/cody-client/result/FbSAQhtzuid/trace/96XAibG4QyM?fields[]=s_name&fields[]=s_serviceName&span=aded4798e8db6458 I’m very happy with this now. Let's try to enable this for client traces to Honeycomb and see how far we get by manually following the trace headers into the Cody Gateway traces |
|
@valerybugakov This one is ready to review now for reals :D |
|
Accompanying OTel collector change here: https://github.com/sourcegraph/deploy-sourcegraph-cloud/pull/18413 |
|
The OTel collector changes are deployed, we can actually test this against dotcom now 😎 |
| } | ||
|
|
||
| return handleSuccess(response) | ||
| } catch (error) { | ||
| return catchError(error) | ||
| } finally { | ||
| span.end() |
There was a problem hiding this comment.
would be called synchronously after creating the promise but before it resolved, which left all spans be very short
TY for catching this! Makes sense.
| @@ -519,6 +520,9 @@ export function suggested(id: CompletionLogID): void { | |||
| if (!event.suggestedAt) { | |||
| event.suggestedAt = performance.now() | |||
|
|
|||
| span?.setAttributes(getSharedParams(event) as any) | |||
There was a problem hiding this comment.
Same as #3034 (comment)
|
|
||
| return tracer.startActiveSpan( | ||
| `POST ${url}`, | ||
| async function* (span): CompletionResponseGenerator { |
There was a problem hiding this comment.
Do we want similar changes in vscode/src/completions/client.ts?
| // Disable default process logging. We do not care about the VS Code extension process | ||
| autoDetectResources: false, |
This PR improves the client site traces for autocomplete and makes it ready to be propagated to Honeycomb. An example trace can be found here: https://ui.honeycomb.io/sourcegraph/environments/cody/datasets/cody-client/result/z87pN9yLSan/trace/rv3ajXLDNV7?fields[]=s_name&fields[]=s_serviceName&span=2edb453047488acc
The changes can be summarized as:
These changes were tested with the following OTel collector processor config:
Test plan
OpenTelemetryService.node.tsremove the dev modespanProcessor.