docs(observability/metrics): document the metrics package by xytan0056 · Pull Request #158 · uber/tango

xytan0056 · 2026-07-07T23:24:38Z

add Tango Metrics Inventory RFC into the repo. Covers the Emitter contract and its usage in callsites

sbalabanov

Missing pieces:

error classification integration
working with histograms for both status and duration and an example on custom buckets

sbalabanov · 2026-07-08T22:58:36Z

+
+Tango is a library. Consumers wire it into their own `fx` graph with their own `tally.Scope`, and often deploy multiple flavors (long-running server, batch job, one-shot CLI) side-by-side. Without a shared convention every deployment invents its own metric names and tag layout, and dashboards devolve into per-name merges.
+
+This package owns:


should this file be README.md under observability/metrics then?

following https://github.com/uber-go/fx/tree/d5da5b04ac906bfbad8b400baeee9b970c1be6f3/docs/src
this is meant to be usage doc

…local metrics Rework metrics.md around the revised proposal: a concrete Tally-backed Emitter that binds and returns instruments, explicit dependency injection instead of context propagation, domain-local operation/metric names and bucket policies, controller-boundary request classification, per-series in-flight counters, and explicit no-op construction.

…ing-method errors Refine the revised metrics design: remove the gauge/in-flight tracking, drop the central shared-conventions vocabulary, and return instruments directly from the binding methods (only New surfaces a nil-scope wiring error). Add a Request-specific tags example.

xytan0056 · 2026-07-15T04:49:52Z

updated based on discussion offline.

…Params Show per-component buckets as a default owned by the package, overridable through the component's Params (zero value falls back), and add the requestMetrics constructor with the same default-and-override handling.

…ord call Collapse the Begin/Complete/requestAttempt two-phase into one deferred Record(start, err); fold the duplicated GetChangedTargets handler into a single example; align the request counter name on 'called'.

… Begin/Complete Begin increments called and returns the start time; Complete records duration and outcome. Keeps the entry-vs-exit ordering without the requestAttempt handle or sync.Once.

… Record called was redundant with succeeded + failed once Complete is deferred, so drop it and the two-phase Begin/Complete in favor of a single deferred Record(start, err).

…uest binding Build requestMetrics per request from a repo-tagged emitter so succeeded/failed/duration break down by repo; rely on tally name+tags caching to bind each (op, repo) series once. Trim request-specific tags to the cardinality rules.

…nd-once claims Make newQueryMetrics take positional emitter+buckets (drop exported MetricParams) to match newRequestMetrics, and soften the two bound-once statements so they don't over-generalize past the per-request request metrics.

xytan0056 · 2026-07-15T07:16:53Z

+    e := c.emitter.Tagged(map[string]string{
+        tagRepo: common.ToShortRemote(req.GetFirstRevision().GetRemote()),
+    })
+    m := newRequestMetrics(e, opGetChangedTargets, c.requestDurationBuckets)


requestmetrics need to be per-repo, so this is initialized per request.

Is it really needed to create a metrics structure per each metric type emitted?
SubmitQueue uses simpler structure with standalone helper functions and a general scope object:
https://github.com/uber/submitqueue/blob/main/submitqueue/orchestrator/controller/start/start.go#L76-L77
https://github.com/uber/submitqueue/blob/main/submitqueue/orchestrator/controller/validate/validate.go#L92-L93

there's a concern of scopes being tagged everywhere which is expensive.
I updated with a memoization approach in each domain, what do you think?

sbalabanov · 2026-07-15T18:11:29Z

+// New creates an emitter for scope. A nil scope is a configuration error.
+func New(scope tally.Scope) (*Emitter, error)
+
+// Nop creates an explicitly disabled emitter.


how would it do so if Emitter is not an interface? With noop tally scope?

Yes

func Nop() *Emitter { return &Emitter{scope: tally.NoopScope} }

sbalabanov · 2026-07-15T18:36:37Z

+    e := c.emitter.Tagged(map[string]string{
+        tagRepo: common.ToShortRemote(req.GetFirstRevision().GetRemote()),
+    })
+    m := newRequestMetrics(e, opGetChangedTargets, c.requestDurationBuckets)


Is it really needed to create a metrics structure per each metric type emitted?
SubmitQueue uses simpler structure with standalone helper functions and a general scope object:
https://github.com/uber/submitqueue/blob/main/submitqueue/orchestrator/controller/start/start.go#L76-L77
https://github.com/uber/submitqueue/blob/main/submitqueue/orchestrator/controller/validate/validate.go#L92-L93

…ency Address review feedback on the metrics design: - Record outcomes as one result-tagged latency histogram instead of separate success/failure counters; counts derive from the histogram. - Add a begin/complete request pattern (started counter at Begin, result- tagged latency histogram at Complete). - Restore shared vocabulary constants (operation names, tag keys, result values) in names.go so outcomes tag consistently across operations. - Fill in concise Emitter method bodies and note Nop wraps NoopScope.

…ain memoization - Move Begin/Complete out of the metrics package into each domain struct (graphRunnerMetrics, requestMetrics) — memoization is domain-local. - Metrics package exports Emitter + constants + Outcome() only. - Standardize start/finish naming convention across all operations. - Domain structs pre-build result-tagged emitters at construction; controller memoizes per (repo, result) pair. - Buckets declared at each callsite, not as package defaults. - Add ResultCancelled for context.Canceled/DeadlineExceeded. - Add M3QL query examples for start rate, outcome counts, P95 latency.

…egin/Complete - Switch domain-local example from graphrunner to orchestrator - Emitter.Tagged is stateless; memoization is per-domain struct - Each domain owns Begin/Complete, op struct, and tagged emitter cache - Metrics package exports only Emitter + constants + Outcome() - Orchestrator example shows repo-memoized Begin/Complete pattern

…pretation

…-request Tagged

…delegation

xytan0056 force-pushed the pr0-docs branch 5 times, most recently from ea9e0ef to d01c936 Compare July 7, 2026 23:37

xytan0056 marked this pull request as ready for review July 7, 2026 23:39

xytan0056 requested review from a team as code owners July 7, 2026 23:39

xytan0056 commented Jul 7, 2026

View reviewed changes

Comment thread docs/observability/metrics.md Outdated

xytan0056 force-pushed the pr0-docs branch 4 times, most recently from a1249f6 to cf7326b Compare July 7, 2026 23:58

sywhang reviewed Jul 8, 2026

View reviewed changes

xytan0056 force-pushed the pr0-docs branch 6 times, most recently from 4f4387b to 54a049f Compare July 8, 2026 21:47

sbalabanov requested changes Jul 8, 2026

View reviewed changes

sbalabanov reviewed Jul 8, 2026

View reviewed changes

Comment thread docs/observability/metrics.md Outdated

xytan0056 force-pushed the pr0-docs branch from 54a049f to 763f930 Compare July 9, 2026 01:06

xytan0056 assigned gh-haniloza Jul 9, 2026

xytan0056 force-pushed the pr0-docs branch from 763f930 to 59d8c93 Compare July 9, 2026 03:59

xytan0056 unassigned gh-haniloza Jul 9, 2026

xytan0056 mentioned this pull request Jul 11, 2026

feat(observability/metrics): add Emitter, options, buckets, and label vocabulary #181

Open

xytan0056 force-pushed the pr0-docs branch from 2a4d0f3 to 1b85a19 Compare July 14, 2026 17:24

docs(observability/metrics): document the metrics package

86797e4

xytan0056 and others added 4 commits July 14, 2026 18:07

Update metrics.md

1e63ef2

Update metrics.md

782dc80

xytan0056 force-pushed the pr0-docs branch from 4371e62 to f6e4228 Compare July 15, 2026 05:19

xytan0056 added 5 commits July 14, 2026 22:50

docs(observability/metrics): restore called-at-entry with lightweight…

66db37a

… Begin/Complete Begin increments called and returns the start time; Complete records duration and outcome. Keeps the entry-vs-exit ordering without the requestAttempt handle or sync.Once.

docs(observability/metrics): drop called, collapse request metrics to…

7816043

… Record called was redundant with succeeded + failed once Complete is deferred, so drop it and the two-phase Begin/Complete in favor of a single deferred Record(start, err).

xytan0056 commented Jul 15, 2026

View reviewed changes

Comment thread docs/observability/metrics.md Outdated

sbalabanov requested changes Jul 15, 2026

View reviewed changes

xytan0056 added 6 commits July 15, 2026 15:33

docs(observability/metrics): trim verbose prose, leave room for inter…

8b8a64b

…pretation

docs(observability/metrics): pre-bind instruments per repo, avoid per…

e212db5

…-request Tagged

docs(observability/metrics): fix Emitter description to match Tagged …

aae9340

…delegation


		Tango is a library. Consumers wire it into their own `fx` graph with their own `tally.Scope`, and often deploy multiple flavors (long-running server, batch job, one-shot CLI) side-by-side. Without a shared convention every deployment invents its own metric names and tag layout, and dashboards devolve into per-name merges.

		This package owns:

Uh oh!

Conversation

xytan0056 commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbalabanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xytan0056 commented Jul 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xytan0056 commented Jul 7, 2026 •

edited

Loading