Add shell completion telemetry coverage#635
Conversation
There was a problem hiding this comment.
Pull request overview
Adds telemetry instrumentation to the interactive shell’s completion system so completion acceptance and suggestion surfaces (Tab, ghost text, completion list, closing-brackets) are tracked consistently alongside existing completion telemetry.
Changes:
- Adds
'shell'as aCompletionSourcein the shared completion telemetry categories. - Instruments shell completion acceptance into the accumulating
completion.acceptedevent (withsrc_shellandtrigger_tab/trigger_ghostTextmeasurements). - Adds new accumulating shell events for ghost text, completion list display, and closing-brackets ghost suggestions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/telemetry/completionCategories.ts | Extends shared completion telemetry source taxonomy to include the shell surface. |
| src/documentdb/shell/DocumentDBShellPty.ts | Adds accumulating telemetry emission for shell completion acceptance, ghost text shown/accepted, completion list shown, and closing-brackets suggestions. |
Comments suppressed due to low confidence (1)
src/documentdb/shell/DocumentDBShellPty.ts:1139
- Same overcounting issue as completion ghost text:
trackClosingBracketsShown()is called even whenShellGhostText.show(closing)doesn’t re-render because the same closing string is already visible. That will inflateshell.closingBrackets.shownand distort the accepted ratio. Gate the telemetry on an actual change in the ghost text (or haveShellGhostText.show()report whether it rendered).
const closing = getClosingBrackets(buffer);
if (closing.length > 0) {
this._ghostTextIsHint = false;
this._ghostTextIsClosingBrackets = true;
this._ghostCandidateKind = undefined;
this._ghostText.show(closing, (d) => this._writeEmitter.fire(d));
this.trackClosingBracketsShown();
return;
|
Independent finding fixed: Single-candidate Tab acceptance counted even when Tab inserts nothing.
Fixed in |
tnaum-ms
left a comment
There was a problem hiding this comment.
Review after fixes applied:
Three issues addressed in this round:
- Ghost/closing-brackets
shownovercount (Medium → Fixed in33ecdafb) —ShellGhostText.show()now returnsboolean; telemetry gated on actual render. - Tab acceptance fires on no-op (Medium → Fixed in
3f40929c) —trackCompletionAccepted()moved afterinsertText()/replaceText(). candidateCountremoved from accumulating telemetry (Fixed in3f40929c) — accumulating telemetry is for occurrence counts; summingcandidates.lengthis not meaningful here. If per-display distribution is needed in the future, it should use non-accumulating telemetry or a min/max/median collector.
One remaining low-severity nit (file comment): inconsistent state reset across _ghostText.clear() call sites. Harmless but worth consolidating into a helper.
All 1937 tests pass. Build succeeds.
ShellGhostText.show() returns early when the same text is already visible, but trackCompletionGhostShown() and trackClosingBracketsShown() fired unconditionally after the call. This inflated shown counts. Changed show() to return a boolean indicating whether it rendered new content, and gate the telemetry calls on that return value.
applySingleCompletion() recorded acceptance telemetry before verifying the Tab press changed the buffer. Fully typed inputs (e.g., 'use' matching candidate 'use') produced empty remaining text but still incremented completion.accepted, inflating acceptance counts. Moved trackCompletionAccepted() to fire only after a successful insert or replace. Also removed candidateCount from shell.completionList accumulating telemetry — accumulating telemetry is designed to count how often events occur (shown/accepted), not to sum variable-length values like candidate list sizes.
All ghost text clear sites now use a single clearGhostState() method that resets _ghostText, _ghostTextIsHint, _ghostTextIsClosingBrackets, and _ghostCandidateKind together. Previously, several call sites only partially reset state fields, which was harmless but inconsistent and fragile for future additions.
✅ Code Quality Checks
This comment is updated automatically on each push. |
📦 Build Size Report
Download artifact · updated automatically on each push. |
Adds telemetry instrumentation to the interactive shell's completion system, closing the gap where Tab completions, ghost text, and closing-bracket suggestions were untracked.
Changes
completion.accepted— shell completions now flow into the existing accumulating event withsrc_shell, alongside playground and collection view. Includestrigger_tab/trigger_ghostTextto distinguish acceptance method.shell.completionGhost— tracks shown vs accepted ratio for inline ghost text suggestions, broken down by category (field,operator,collectionName, etc.).shell.completionList— tracks when the multi-match picker is displayed (e.g.,db.col.fin⇥showing available methods), broken down by category.shell.closingBrackets— tracks shown vs accepted ratio for closing-bracket ghost text (}})suggestions).'shell'as aCompletionSourceincompletionCategories.ts.All events use
callWithAccumulatingTelemetryfor batched, low-overhead emission.Telemetry correctness fixes (added during review)
ShellGhostText.show()now returnsboolean;showntelemetry is gated on actual render to prevent overcount when the same ghost text stays visible.completion.acceptedis only emitted after a successful insert or replace — no-op Tab presses (fully typed word matching a single candidate) no longer inflate acceptance counts.candidateCountwas removed fromshell.completionList:callWithAccumulatingTelemetrysums measurements across the batch, making a per-display length value analytically misleading. Tracked as #636 for future min/max/median support.