fix(telemetry): deliver the usage event reliably and cheaply

Description

addTelemetryHook spawned sendTelemetryData in a goroutine that was never joined, so an event only landed if something else kept the process alive after Execute() returned.

In practice that something is checkForUpdate's network call. ShouldSkipUpdate skips it for exactly check-update, completion, git-credential, credential-helper, whatsnew and version — which is the list of commands missing from the gitlab_cli_command_used stream. Every other command is undercounted by however often the update check happened to run.

So the loss is broader than #8526 describes: it is not confined to completion and version, and it makes all per-command volumes on the dashboard undercounts of an unknown factor.

Built binary against a local instance, same config, same commands:

command before after
glab version no event sent
glab completion no event sent
glab whatsnew no event sent
glab api /user no event sent
glab repo view no event sent

0 of 5 before, 5 of 5 after.

The fix

The send is now synchronous, bounded by a 2s context deadline that covers everything it does. There is no goroutine, which is what the deadline made redundant: the context already cancels the request, so a select only duplicated it. That also leaves the hook with no concurrency at all, retiring the #7885 (closed) race class outright rather than narrowing it.

The send is deliberately detached from cmd.Context(), which is already cancelled when the command was interrupted.

Making it cheap

Review raised that this puts a blocking network call on commands ShouldSkipUpdate deliberately keeps fast. Three changes address that, each measured rather than assumed.

Retries were the dominant cost. The client defaults to RetryMax: 5 with backoff, so a best-effort event was retrying failures and turning an instant one into a slow one. Telemetry now sends with a no-op CheckRetry:

scenario before after
connection refused (instance down, stale api_host) 2.10s 0.07s
DNS failure 0.08s 0.07s
blackholed host (VPN/firewall) 2.08s 2.10s

Refused is the ordinary self-managed failure and now costs nothing. Only a host that silently drops packets still reaches the ceiling, which nothing can avoid.

Machine-invoked commands are skipped: completion, auth git-credential, auth credential-helper. completion is evaluated from shell startup files and the credential helpers are invoked by git on every authenticated operation. It is not only latency — counting them measures shell spawns and git operations rather than anything a user chose to do. #8526 used completion's zero events as evidence of the delivery bug, which the fix itself now demonstrates.

Related: ShouldSkipUpdate matches its git-credential and credential-helper entries against expandedArgs[0], which is auth for both since they are auth subcommands, so those entries never fire. This MR's skip list keys on the full command path instead. Worth a separate issue.

The project is no longer fetched. Attaching project_id meant an API round trip per in-repo command. POST /usage_data/track_event accepts project_path and resolves it server side, and glab already knows the path for free. Verified against gitlab.com: sending both yields {"error":"project_id, project_path are mutually exclusive"}, and project_path alone is accepted. process_event uses the path only to look the IDs up and does not forward it, so nothing new reaches Snowplow.

Net effect:

before after
glab version, in a repo 0.42s 0.25s
glab completion 0.28s 0.05s (floor is 0.06s)
glab version, outside a repo 0.28s 0.24s
floor, telemetry disabled 0.06s 0.06s

Two things worth flagging

Telemetry failures drop from LogErrorf to dbg.Debug. They almost never surfaced while the goroutine was being killed. Now that the send completes, anyone offline, behind a proxy, or on an instance without the endpoint would otherwise see Could not send telemetry data: ... on stderr after every command.

The request is hand-rolled. client-go's TrackEventOptions has no ProjectPath field, so the send uses the exported NewRequest/Do rather than the typed UsageData service. Worth adding upstream so this can go back to the typed helper.

Resolves #8526 Related #7885 (closed), #8499

How has this been tested?

Because the hand-rolled request bypasses the generated service mocks, the tests drive a real httptest server and assert the wire format — method, path, and the encoded body. For a hand-rolled request that is the thing worth checking, and it caught that an empty project_path must be omitted rather than sent as "", given the mutual exclusion with project_id.

Also covered: the hook does not return until the event is sent (the original regression), the request carries a deadline, noRetry never retries but still surfaces context cancellation, and no event is sent for any of the three machine-invoked commands.

go test -race ./cmd/glab/..., the full suite, make generate and make lint all pass, plus the end-to-end runs above against a local instance and the latency measurements against gitlab.com.

Edited by Kai Armstrong

Merge request reports

Loading
Loading