telemetry: add best-effort usage telemetry recorder - #1378
BolajiOlajide wants to merge 6 commits into
Conversation
Add the internal/telemetry package: a Recorder that records src-cli usage events to the authenticated Sourcegraph instance via the Telemetry V2 recordEvents GraphQL mutation. Recording is strictly best-effort — network, GraphQL, timeout, and old-instance (<5.2) failures are all swallowed and never affect the command the user ran. Command identity is encoded in feature/action with numeric-only metadata (no user content, no privateMetadata), and names are validated against Sourcegraph's server-side naming rules.
Telemetry must never contaminate command output, but GraphQL request execution can print OAuth reauthorization guidance on HTTP 401 responses. Send telemetry through the authenticated HTTP client directly so failures remain best-effort without interactive output, while preserving authentication and cancellation. Add regression coverage for OAuth failures, request shape, GraphQL errors, and timeout cancellation. ## Test Plan - go test ./... - go test ./internal/telemetry -run 'TestRecord_(AppliesTimeout|TimeoutCancelsHTTPRequest)' -count=20
The telemetry API already validates feature and action names authoritatively, so maintaining a second validator in src-cli adds drift risk without improving correctness. Remove the duplicate validation and allow rejected events to follow the recorder's existing best-effort failure path. ## Test Plan - go test ./...
The telemetry package has no production callers yet, so exporting event models, source configuration, options, and test-only controls commits src-cli to an API before its integration requirements are known. Keep only the recorder construction and recording operations public, fix the client identity internally, and pass event values directly. ## Test Plan - go test ./...
| // eventParametersVersion is the schema version of the metadata we attach to | ||
| // each event. Bump it when the shape of the metadata changes. | ||
| eventParametersVersion = 1 |
There was a problem hiding this comment.
this assumes individual src CLI telemetry callsites will never provide their own parameters, is that the case?
I think the telemetry SDK concern should be separated from "telemetry middleware" etc
There was a problem hiding this comment.
This is a required field from the telemetry GraphQL contract. You can check it out here: https://github.com/sourcegraph/sourcegraph/blob/7b2c944fffa65c5e60c35685806f4b985d140637/cmd/frontend/graphqlbackend/telemetry.graphql#L205-L239
There was a problem hiding this comment.
@BolajiOlajide yes, but in the SDK implementations, individual telemetry callsites provide this parameter to self-describe the callsite. In your implementation, it is a global property for all telemetry callsites
There was a problem hiding this comment.
The docstring on that field indicates that it is meant to be provided on a per-event, not per-integration basis:
Version of the event parameters, used for indicating the "shape" of this
event's metadata.
| // defaultTimeout bounds how long a single Record call may spend recording. | ||
| // It is deliberately short: telemetry is sent synchronously right before the | ||
| // process exits, so it must not add meaningful latency. | ||
| defaultTimeout = 2 * time.Second |
There was a problem hiding this comment.
can we have telemetry be recorded in the background and have process wait for completion - up to deadline - when it receives a shutdown signal? I think we do something like that in sg
There was a problem hiding this comment.
I'll have this as a separate ticket.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hm, given synchronous telemetry submission will directly impact the day-to-day usage of src CLI as soon as we release this, should we hold a higher bar for the first implementation?
There was a problem hiding this comment.
I wanted this to focus on just the client alone. The graceful flush and shutdown are technically next and I have a draft locally alreadt. They touch the internals of the CLI framework, hence why I am pushing to a separate PR so it's easily reviewable.
There was a problem hiding this comment.
Also, the recorder isn't hooked up anywhere yet also, thats why I'm separating that also.
Keep expected telemetry failures out of normal output while making them available through debug logging with the underlying error. Narrow the concrete recorder type, require its logger dependency, and retain the server-required parameters and numeric metadata wire format. ## Test Plan - go test ./...
|
@BolajiOlajide could we also support adding |
akalia25
left a comment
akalia25
left a comment
There was a problem hiding this comment.
LGTM! Just two notes:
- is the two second timeout enough
- can we extend to include sending
privateMetadataas well?
Thanks for adding this!
|
3 participants
Closes CPL-1005
src-cli needs a reusable way to report usage without telemetry failures affecting command behavior or output. This adds an internal Telemetry V2 recorder that sends events to the authenticated Sourcegraph instance with a bounded two-second timeout.
Events use the server-required
parameters.versionand numeric, PII-free metadata shape. The recorder relies on authoritative server-side event validation, preserves the existing API client authentication and transport behavior, and uses the HTTP-level client so OAuth failures cannot print interactive guidance into command output. Network, GraphQL, timeout, and unsupported-instance failures remain best-effort and are logged only at debug level.This PR provides the foundational package only. Command wiring and opt-out or suppression behavior will land separately.
Test Plan
go test ./...