microsoft / microsoft/vscode-documentdb
[telemetry] Add runWithAccumulatingTelemetry: wrap hot-path actions with batched success / immediate-failure telemetry
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 31
- Forks
- 22
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 21
Description
Summary
Add a runWithAccumulatingTelemetry helper (working name) that wraps an action — runs it, times it, and records the outcome — but instead of emitting one telemetry event per call, folds successes into the existing batched/accumulated rollup and emits failures immediately. This is the "instrument a hot code path" companion to the existing accumulateTelemetry (which only records a data point and does not run your work).
Specced and deferred out of PR #766 (see the R766-P07 note in docs/ai-and-plans/PRs/766-webview-ext-package-redesign/webview-ext-review.md, Iteration 11). Not urgent — file now so it's easy to pick up when demand for richer hot-path telemetry arises.
Why
Today there are two extremes for instrumenting code:
callWithTelemetryAndErrorHandling— one backend event per call. Correct for discrete user actions, but on hot paths (per webview RPC, per keystroke/completion, per scroll/paging op) it is too expensive and too noisy, so those paths tend to be left uninstrumented.accumulateTelemetry(post-PR-766) — cheap, batched aggregate, but it does not run your work; you can only hand it a sample bag to fill. You can't "wrap an action" with it.
There is no middle option that lets you wrap real (async) work on a hot path and get duration distribution + success/failure counts for near-free. runWithAccumulatingTelemetry fills that gap.
Reference: the base API has no debounce/throttle/sampling
Confirmed while reviewing @microsoft/vscode-azext-utils: callWithTelemetryAndErrorHandling has no rate-limiting — the only "don't send" levers are the binary suppressAll / suppressIfSuccessful flags, and every non-suppressed call pays full per-call machinery (context allocation, AzExtUserInput construction, handler loops, per-property masking regex, reporter dispatch). Reducing event volume on hot paths is therefore our responsibility, which is what the accumulating family is for.
Proposed design
export async function runWithAccumulatingTelemetry<T>(
callbackId: string,
action: (sample: TelemetrySample) => T | PromiseLike<T>,
options?: AccumulateTelemetryOptions,
): Promise<T>;
| Outcome | Behavior | Emit path |
|---|---|---|
| Success | Time the action; fold auto_duration_ms + any sample.* the action set into the batch (reuse accumulate/flushState). Return the action's value. |
Deferred — rolled-up event on flush, result=Succeeded. |
| Failure | Copy the partial sample onto a failure event, attach the error (type/message/stack/masking), then rethrow. Not folded into the batch. |
Immediate — one event, result=Failed. |
| Cancellation | Same immediate path; UserCancelledError auto-classified result=Canceled. Rethrown. |
Immediate, Canceled. |
Core design rule
Errors escape the reduction. Successes (cheap, high-volume) are batched; failures/cancellations (rare, high-value) emit immediately and in full. You almost never want to lose or delay a failure; losing the 10,000th successful completion is fine.
Design points to ratify at implementation time
- Rethrows on error (returns
Promise<T>, notT | undefined). Intentional divergence fromcallWithTelemetryAndErrorHandling's swallow-default: a work-wrapper must be transparent so the caller still gets the value and still sees exceptions. Document the divergence prominently. - Action receives the
sampleso it can annotate its own measurements (e.g.sample.measurements.rowCount = n) alongside doing work — mirrors howcallWithTelemetryAndErrorHandlingpassesctx. On failure, whatever was set before the throw is emitted as failure context. - No explicit success counter needed initially —
dist_auto_duration_ms_countalready gives the number of batched successes; failures are countable as their own rows. (Could add explicitruns/failuresmeasurements later if failure-rate dashboards want them under one event.) - Async only to start; a sync variant can follow if a real need appears.
- Reuse the existing internals —
TelemetrySample,getOrCreateState,accumulate,flushState,AUTO_DURATION_DISTRIBUTION_KEY, and the same immediate-error-emit pattern already inaccumulateTelemetry. This keeps it a thin layer, not a parallel implementation. - Schema note: successes and failures share the event name (
callbackId) but differ byresult— success rollup carriesdist_*; failures carryerror/errorMessage. Coherent and filterable.
Naming rationale
The verb should encode whether the callback runs:
accumulateTelemetry(id, sample => …)— does not run your work (records a data point).runWithAccumulatingTelemetry(id, sample => action)— does run your work, so arunWith…name is honest here (unlike the oldcallWithAccumulatingTelemetry, which is why it was renamed in PR #766).
Acceptance criteria
-
runWithAccumulatingTelemetry<T>implemented insrc/utils/accumulatingTelemetry.ts, reusing existing internals. - Success → duration folded into batch, value returned, not emitted until flush.
- Success path never enters
callWithTelemetryAndErrorHandling. - Failure → emitted immediately once (
result=Failed, error attached) and rethrown. - Cancellation → classified
Canceled, rethrown. - Partial
sampleset before a throw appears on the failure event. - Unit tests covering all of the above.
- (Optional but recommended) dogfood on one real hot path (e.g. a shell completion tracker or an RPC procedure) so it ships with a genuine first consumer.
- Docs: JSDoc on the helper; consider adding an accumulating-telemetry section to the
telemetry-instrumentationskill showing bothaccumulateTelemetryandrunWithAccumulatingTelemetry. - Standard gate:
prettier-fix·lint·jest·build(+l10nif any strings).
Context / links
- PR #766 (redesign) — Iterations 8–11 in
webview-ext-review.mdcover the latency analysis (R766-P05), the console gate (Iteration 9), the accumulate/flush split (Iteration 10), and the rename (Iteration 11). - This work is R766-P07.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with src/utils/accumulatingTelemetry.ts and read the existing TelemetrySample, getOrCreateState, accumulate, flushState, and immediate-error handling paths. Add the async wrapper and unit tests for success, failure, cancellation, partial samples, rethrowing, and deferred flushing. Run prettier-fix, lint, jest, and build; done means all acceptance criteria pass without routing successes through callWithTelemetryAndErrorHandling.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- observability, tooling
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100