microsoft / microsoft/vscode-documentdb

[telemetry] Add runWithAccumulatingTelemetry: wrap hot-path actions with batched success / immediate-failure telemetry

Open
#777 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
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:

  1. callWithTelemetryAndErrorHandlingone 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.
  2. 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>, not T | undefined). Intentional divergence from callWithTelemetryAndErrorHandling'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 sample so it can annotate its own measurements (e.g. sample.measurements.rowCount = n) alongside doing work — mirrors how callWithTelemetryAndErrorHandling passes ctx. On failure, whatever was set before the throw is emitted as failure context.
  • No explicit success counter needed initiallydist_auto_duration_ms_count already gives the number of batched successes; failures are countable as their own rows. (Could add explicit runs/failures measurements 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 internalsTelemetrySample, getOrCreateState, accumulate, flushState, AUTO_DURATION_DISTRIBUTION_KEY, and the same immediate-error-emit pattern already in accumulateTelemetry. This keeps it a thin layer, not a parallel implementation.
  • Schema note: successes and failures share the event name (callbackId) but differ by result — success rollup carries dist_*; failures carry error/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 a runWith… name is honest here (unlike the old callWithAccumulatingTelemetry, which is why it was renamed in PR #766).

Acceptance criteria

  • runWithAccumulatingTelemetry<T> implemented in src/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 sample set 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-instrumentation skill showing both accumulateTelemetry and runWithAccumulatingTelemetry.
  • Standard gate: prettier-fix · lint · jest · build (+ l10n if any strings).

Context / links

  • PR #766 (redesign) — Iterations 8–11 in webview-ext-review.md cover 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.