-
Notifications
You must be signed in to change notification settings - Fork 76
telemetry: add best-effort usage telemetry recorder #1378
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
base: main
Are you sure you want to change the base?
Changes from all commits
46ecea8
745a8a0
3b460b9
12e947e
1574f38
3637d1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| // Package telemetry records src-cli usage events to the Sourcegraph instance | ||
| // the CLI is authenticated to, using the Telemetry V2 `recordEvents` GraphQL | ||
| // mutation. | ||
| // | ||
| // Recording is strictly best-effort: a failure to record — whether a network | ||
| // error, a GraphQL error, a timeout, or an instance too old to support the | ||
| // mutation — must never affect the command the user actually ran. See | ||
| // .context/TELEMETRY.md for the design and event schema. | ||
| package telemetry | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "net/http" | ||
| "sort" | ||
| "time" | ||
|
|
||
| "github.com/sourcegraph/src-cli/internal/api" | ||
|
|
||
| "github.com/sourcegraph/log" | ||
| "github.com/sourcegraph/sourcegraph/lib/errors" | ||
| ) | ||
|
|
||
| const ( | ||
| // clientName identifies src-cli as the source of telemetry events. | ||
| clientName = "src.cli" | ||
|
|
||
| // eventParametersVersion is the schema version of the metadata we attach to | ||
| // each event. Bump it when the shape of the metadata changes. | ||
| eventParametersVersion = 1 | ||
|
|
||
| // 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 | ||
|
Comment on lines
+33
to
+36
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll have this as a separate ticket.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, given synchronous telemetry submission will directly impact the day-to-day usage of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the recorder isn't hooked up anywhere yet also, thats why I'm separating that also. |
||
| ) | ||
|
|
||
| // recordEventsMutation mirrors the mutation used by Sourcegraph's own clients. | ||
| // The `telemetry` mutation only exists on Sourcegraph 5.2+, so on older | ||
| // instances this returns GraphQL errors, which Record silently drops. | ||
| const recordEventsMutation = `mutation RecordTelemetryEvents($events: [TelemetryEventInput!]!) { | ||
| telemetry { | ||
| recordEvents(events: $events) { | ||
| alwaysNil | ||
| } | ||
| } | ||
| }` | ||
|
|
||
| // Recorder records events through an api.Client. | ||
| type recorder struct { | ||
| client api.Client | ||
| clientVersion string | ||
| timeout time.Duration | ||
| logger log.Logger | ||
| } | ||
|
|
||
| // NewRecorder returns a Recorder for the given src-cli version. | ||
| func NewRecorder(client api.Client, logger log.Logger, clientVersion string) *recorder { | ||
| return &recorder{ | ||
| client: client, | ||
| clientVersion: clientVersion, | ||
| timeout: defaultTimeout, | ||
| logger: logger, | ||
| } | ||
| } | ||
|
|
||
| // Record sends an event on a best-effort basis. Feature and action identify the | ||
| // event (for example, "srcCli.search" and "succeeded"). Metadata must contain | ||
| // only numeric, PII-free facts. Private metadata may contain arbitrary JSON and | ||
| // is not exported from Sourcegraph instances by default. Network, GraphQL, | ||
| // timeout, and old-instance failures are logged at debug level. Record applies | ||
| // its own timeout, so the caller's context need not carry a deadline. | ||
| func (r *recorder) Record(ctx context.Context, feature, action string, metadata map[string]float64, privateMetadata map[string]any) { | ||
| if err := r.record(ctx, feature, action, metadata, privateMetadata); err != nil { | ||
| r.logger.Debug("recording telemetry event", log.String("feature", feature), log.String("action", action), log.Error(err)) | ||
| } | ||
| } | ||
|
|
||
| // record does the work behind Record and returns any error, so it can be tested | ||
| // directly. Callers outside tests should use Record. | ||
| func (r *recorder) record(ctx context.Context, feature, action string, metadata map[string]float64, privateMetadata map[string]any) error { | ||
| ctx, cancel := context.WithTimeout(ctx, r.timeout) | ||
| defer cancel() | ||
|
|
||
| vars := map[string]any{ | ||
| "events": []any{buildEventInput(r.clientVersion, feature, action, metadata, privateMetadata)}, | ||
| } | ||
|
|
||
| payload, err := json.Marshal(map[string]any{ | ||
| "query": recordEventsMutation, | ||
| "variables": vars, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Use the HTTP-level client because GraphQL Request.Do may print interactive | ||
| // authentication guidance; telemetry must never affect command output. | ||
| req, err := r.client.NewHTTPRequest(ctx, http.MethodPost, ".api/graphql", bytes.NewReader(payload)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| resp, err := r.client.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return errors.Newf("telemetry request failed: %s", resp.Status) | ||
| } | ||
| var result struct { | ||
| Errors []json.RawMessage `json:"errors"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | ||
| return err | ||
| } | ||
| if len(result.Errors) > 0 { | ||
| return api.NewGraphQlErrors(result.Errors) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // buildEventInput builds a single TelemetryEventInput as a JSON-serializable map. | ||
| func buildEventInput(clientVersion, feature, action string, metadata map[string]float64, privateMetadata map[string]any) map[string]any { | ||
| parameters := map[string]any{ | ||
| "version": eventParametersVersion, | ||
| "metadata": buildMetadata(metadata), | ||
| } | ||
| if privateMetadata != nil { | ||
| parameters["privateMetadata"] = privateMetadata | ||
| } | ||
| return map[string]any{ | ||
| "feature": feature, | ||
| "action": action, | ||
| "source": map[string]string{ | ||
| "client": clientName, | ||
| "clientVersion": clientVersion, | ||
| }, | ||
| "parameters": parameters, | ||
| } | ||
| } | ||
|
|
||
| // buildMetadata converts numeric metadata into the list of {key, value} inputs | ||
| // the API expects, sorted by key for deterministic output. | ||
| func buildMetadata(metadata map[string]float64) []any { | ||
| out := make([]any, 0, len(metadata)) | ||
| keys := make([]string, 0, len(metadata)) | ||
| for key := range metadata { | ||
| keys = append(keys, key) | ||
| } | ||
| sort.Strings(keys) | ||
| for _, key := range keys { | ||
| out = append(out, map[string]any{"key": key, "value": metadata[key]}) | ||
| } | ||
| return out | ||
| } | ||
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.
this assumes individual
srcCLI 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring on that field indicates that it is meant to be provided on a per-event, not per-integration basis: