fix(observability): document real outcome vocabulary, add graphrunner lifecycle metrics (audit #11)

CLAUDE.md

@@ -63,7 +63,7 @@ The top-level split is by **responsibility**, not by domain: `controller/` handl
 The controller is the YARPC service implementation. It owns the transport-adjacent concerns: request validation, response chunking, cancellation handling, fan-out across revisions, and metrics emission. It does **not** own workspace creation, git operations, or graph computation — those belong to the orchestrator and below.
 
 Each RPC method follows the same shape:
-1. Call `metrics.Begin(emitter, op, buckets)` to record a start counter and capture the start time. Defer `op.Complete(err)` which records a finish-duration histogram tagged `result` with the outcome (one of `success`, `cancelled`, `user`, `infra`, `infra_retryable`). On failure, also emit a `failures` counter tagged with `error_code` via `emitFailureMetric`.
+1. Call `metrics.Begin(emitter, op, buckets)` to record a start counter and capture the start time. Defer `op.Complete(err)` which records a finish-duration histogram tagged `result` with the outcome (one of `success`, `cancelled`, `user`, `infra`, `infra_retryable`).
 2. Validate the request; reject with a `TangoError` classified `ErrorUser` on bad input.
 3. Attempt to serve the response from cache (read-through). On a cache miss, drive the orchestrator to compute the target graph(s).
 4. Stream the result to the client.
@@ -82,7 +82,7 @@ The bundled `nativeOrchestrator` (under `orchestrator/native_orchestrator.go`) i
 
 A custom orchestrator satisfies the same `orchestrator.Orchestrator` interface and is wired into the controller in place of the native one. It is the right seam to plug in remote build execution, CI-managed checkouts, or organization-specific caching — the controller and `graphrunner` stay unchanged.
 
-Whichever implementation is used, the orchestrator is responsible for **classifying** errors by wrapping them with `tangoerrors.NewInfra`, `tangoerrors.NewInfraRetryable`, or `tangoerrors.NewUser` (from `core/errors`) so the metrics pipeline can tag failures with a stable `error_code`. Per-cause classifiers in `orchestrator/errors.go` (e.g. `classifyLeaseError`, `classifyGitError`, `classifyBazelClientError`) map component-level sentinels (`repomanager.ErrPoolTimeout`, `git.ErrTimeout`, `bazel.ErrNetwork`) to the appropriate error code.
+Whichever implementation is used, the orchestrator is responsible for **classifying** errors by wrapping them with `tangoerrors.NewInfra`, `tangoerrors.NewInfraRetryable`, or `tangoerrors.NewUser` (from `core/errors`) so the metrics pipeline can tag the finish histogram with a stable `result`. Per-cause classifiers in `orchestrator/errors.go` (e.g. `classifyLeaseError`, `classifyGitError`, `classifyBazelClientError`) map component-level sentinels (`repomanager.ErrPoolTimeout`, `git.ErrTimeout`, `bazel.ErrNetwork`) to the appropriate error code.
 
 ### Graphrunner
 
@@ -226,9 +226,9 @@ Errors are classified by **origin** (user vs infra) for metrics. The contract li
 
 **Key rules:**
 
-1. **Wrap at the failure site** with the appropriate constructor (`NewUser`, `NewInfra`, `NewInfraRetryable`) so the metric tag carries a stable `error_code`. The `GetErrorCode` function extracts the code from any error chain; context cancellations are detected automatically.
+1. **Wrap at the failure site** with the appropriate constructor (`NewUser`, `NewInfra`, `NewInfraRetryable`) so the finish histogram carries a stable `result`. The `GetErrorCode` function extracts the code from any error chain; context cancellations are detected automatically.
 2. **The deepest layer that knows the classification wraps the error.** Lower layers (storage, git, bazel) return plain errors with their own sentinels (`storage.ErrNotFound`, `git.ErrTimeout`, `git.ErrFatal`, `bazel.ErrNetwork`, `repomanager.ErrPoolTimeout`). The orchestrator decides whether a given failure is user-caused or infra-caused — per-cause classifiers in `orchestrator/errors.go` (`classifyLeaseError`, `classifyGitError`, `classifyBazelClientError`) handle this mapping.
-3. **The controller emits the `error_code` metric tag.** `controller/errors.go` provides `emitFailureMetric`, which calls `tangoerrors.GetErrorCode(err).String()` to tag the `failures` counter. The `errors.Fields` helper produces structured zap fields (`error` + `error_code`) for log lines.
+3. **The controller completes the standard lifecycle metric.** `metrics.Op.Complete` derives the finish histogram's `result` tag from `tangoerrors.GetErrorCode(err).String()`. The `errors.Fields` helper separately produces structured zap fields (`error` + `error_code`) for log lines.
 4. **Errors flow through `errors.Is` / `errors.As`** — `TangoError` implements `Unwrap`, so wrapping preserves the underlying sentinels (e.g. callers can still `errors.Is(err, storage.ErrNotFound)` through a `TangoError` wrapper).
 ### Caching and Treehashes
 

controller/errors.go

@@ -15,9 +15,7 @@
 package controller
 
 import (
-	tangoerrors "github.com/uber/tango/core/errors"
 	"github.com/uber/tango/internal/mapper"
-	"github.com/uber/tango/observability/metrics"
 )
 
 // toWireError converts err into a YARPC error carrying a TangoError detail so
@@ -28,11 +26,3 @@ import (
 func toWireError(err error) error {
 	return mapper.ToProtoError(err)
 }
-
-// emitFailureMetric tags the failure counter with err's ErrorCode. e should
-// already carry the repo tag; op is the operation subscope the counter lands under.
-func emitFailureMetric(e *metrics.Emitter, op string, err error) {
-	e.Tagged(map[string]string{
-		"error_code": tangoerrors.GetErrorCode(err).String(),
-	}).Counter(op, "failures").Inc(1)
-}

controller/getchangedtargetgraph.go

@@ -32,7 +32,6 @@ func (c *controller) GetChangedTargetGraph(request *pb.GetChangedTargetGraphRequ
 	)
 	defer func() {
 		op.Complete(retErr)
-		emitFailureMetric(c.emitter, opGetChangedTargetGraph, retErr)
 	}()
 	return retErr
 }

controller/getchangedtargets.go

@@ -60,7 +60,6 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str
 		op.Complete(retErr)
 		if retErr != nil {
 			logger.Error("GetChangedTargets failed", tangoerrors.Fields(retErr)...)
-			emitFailureMetric(e, opGetChangedTargets, retErr)
 			retErr = toWireError(retErr)
 		}
 	}()

controller/gettargetgraph.go

@@ -44,7 +44,6 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb
 		op.Complete(retErr)
 		if retErr != nil {
 			logger.Error("GetTargetGraph failed", tangoerrors.Fields(retErr)...)
-			emitFailureMetric(e, opGetTargetGraph, retErr)
 			retErr = toWireError(retErr)
 		}
 	}()
@@ -127,6 +126,7 @@ func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entit
 			if ctx.Err() != nil {
 				err = context.Cause(ctx)
 			}
+			metrics.RecordCacheLookup(e, opGetTargetGraph, metrics.GraphCacheLookup, err)
 			if err != nil {
 				if !storage.IsNotFound(err) {
 					return nil, fmt.Errorf("graph reader: %w", err)

controller/metrics_test.go

@@ -40,5 +40,4 @@ func TestControllerMetricsPathShape(t *testing.T) {
 	snap := ts.Snapshot()
 	assert.Contains(t, snap.Counters(), "controller.get_changed_target_graph.start+")
 	assert.Contains(t, snap.Histograms(), "controller.get_changed_target_graph.finish+result=infra")
-	assert.Contains(t, snap.Counters(), "controller.get_changed_target_graph.failures+error_code=infra")
 }

docs/observability/metrics.md

@@ -125,43 +125,36 @@ The only tag reused across an operation's metrics is `repo`, so the caller bakes
 
 Metric and operation names are declared by the package that owns each operation, while the *outcome vocabulary* is shared: tag keys and result values live in `metrics/names.go` and every operation draws from them. Buckets are shared too — each callsite passes one of the `Fast`/`Slow`/`LargeCount` sets from `buckets.go` by timescale.
 
-```go
-package metrics
+### Outcome vocabulary
 
-// Tag keys.
-const (
-    TagRepo   = "repo"
-    TagResult = "result"
-)
-
-// Result values for TagResult.
-const (
-    ResultSuccess   = "success"
-    ResultFailure   = "failure"
-    ResultCancelled = "cancelled"
-    ResultHit       = "hit"
-    ResultMiss      = "miss"
-)
-```
+`Outcome(err)` maps an error to a `result` tag value for the `finish` histogram. A nil error is `success`; any non-nil error delegates to `tangoerrors.GetErrorCode(err).String()`, which classifies by `ErrorCode`:
 
-Operation names are *not* centralized here — each consuming package declares its own op-name consts in its `metrics.go`, named after the interface method they measure (e.g. `get_target_graph`, `compute`, `lease`).
-```go
-// Outcome maps an error to a result tag value. Only an explicitly cancelled
-// context (client disconnect or shutdown) is `cancelled`; a deadline exceeded
-// is a genuine timeout and counts as `failure` (tagged infra on the
-// failure_type axis).
-func Outcome(err error) string {
-    switch {
-    case err == nil:
-        return ResultSuccess
-    case errors.Is(err, context.Canceled):
-        return ResultCancelled
-    default:
-        return ResultFailure
-    }
-}
-```
-The `result` tag is the sole outcome signal. Success, failure, and cancelled counts are derived from the `finish` histogram by summing its buckets grouped by `result`.
+| `result` value | When | Source |
+|---|---|---|
+| `success` | `err == nil` | hardcoded |
+| `cancelled` | `errors.Is(err, context.Canceled)` | `ErrorCancelled.String()` |
+| `user` | error wraps a `TangoError` with `ErrorUser` | `ErrorUser.String()` |
+| `infra` | unclassified error or `ErrorInfra` | `ErrorInfra.String()` |
+| `infra_retryable` | error wraps a `TangoError` with `ErrorInfraRetryable` | `ErrorInfraRetryable.String()` |
+
+Note: `"failure"` is **not** a valid outcome value. Dashboards should filter on the concrete values above (`cancelled`, `user`, `infra`, `infra_retryable`) rather than a single `failure` bucket.
+
+A `context.DeadlineExceeded` without a `TangoError` wrapper is classified as `infra` (a genuine timeout), not `cancelled` — only an explicit `context.Canceled` (client disconnect or shutdown) maps to `cancelled`.
+
+Operation names are *not* centralized here — each consuming package declares its own op-name consts in its `metrics.go`, snake_cased after the interface method they measure (e.g. `get_target_graph`, `compute`, `lease`).
+
+### Cache-lookup counters
+
+Cache users may record a lookup counter under their parent operation with
+`RecordCacheLookup(e, parentOp, name, err)`. The caller owns the bounded metric
+name; the shared helper owns the result semantics:
+
+- a nil error emits `result=hit`;
+- a `storage.NotFoundError` emits `result=miss`;
+- any other error emits nothing, because an infrastructure failure is not a
+  cache miss and must not skew the hit rate.
+
+The `result` tag on the `finish` histogram is the primary outcome signal. Success and error-class counts are derived from the `finish` histogram by summing its buckets grouped by `result`.
 
 ## Usage
 
@@ -193,17 +186,13 @@ func (c *controller) GetChangedTargets(req *pb.GetChangedTargetsRequest, stream
 }
 ```
 
-A sub-operation uses `Begin`/`Complete` for the `start`/`finish` duration exactly like the request handlers, reusing the repo-tagged emitter the caller already holds.
+A sub-operation uses `Begin`/`Complete` for the `start`/`finish` duration exactly like the request handlers, reusing the repo-tagged emitter the caller already holds. Cache lookups within an operation can record a result-tagged counter alongside the duration.
 
 ```go
-// opCacheRead is an extension op, declared next to the emit site.
-const opCacheRead = "cache_read"
-
-func (c *controller) readGraphCache(ctx context.Context, e *metrics.Emitter, key string) (_ storage.GraphReader, hit bool, retErr error) {
-    op := metrics.Begin(e, opCacheRead, metrics.FastDurationBuckets)
-    defer func() { op.Complete(retErr) }()
-
-    return c.lookupGraph(ctx, key)
+value, err := cache.Get(ctx, key)
+metrics.RecordCacheLookup(e, parentOp, cacheLookupMetric, err)
+if err == nil {
+    return value, nil
 }
 ```
 
@@ -213,7 +202,7 @@ func (c *controller) readGraphCache(ctx context.Context, e *metrics.Emitter, key
 # operation rate
 fetch service:tango name:controller.get_changed_targets.start
 
-# success / failure / cancelled counts
+# success and classified error counts
 fetch service:tango name:controller.get_changed_targets.finish | sum by (result)
 
 # P95 latency of successful requests
@@ -229,4 +218,3 @@ fetch service:tango name:controller.get_changed_targets.target_count | histogram
 ## Request-specific tags
 
 Each distinct tag value is a new series, so tag values must be bounded — never request IDs, commit hashes, paths, or raw repo URLs. `repo` is safe only with an explicit cardinality budget and a normalized, allow-listed value; the handlers above apply it that way (`ToShortRemote`).
-

graphrunner/native.go

@@ -54,7 +54,10 @@ func NewNativeGraphRunner(p NativeGraphRunnerParams) GraphRunner {
 	}
 }
 
-func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) (targethasher.Result, error) {
+func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) (_ targethasher.Result, retErr error) {
+	op := metrics.Begin(g.emitter, _opCompute, metrics.SlowDurationBuckets)
+	defer func() { op.Complete(retErr) }()
+
 	query := "//external:all-targets + deps(//...:all-targets)"
 	if g.config.ExcludeExternalTargets {
 		query = "deps(//...:all-targets)"

observability/metrics/names.go

@@ -19,8 +19,8 @@ import (
 	"github.com/uber/tango/core/storage"
 )
 
-// Operation (op) names live in each consuming package's metrics.go, named after
-// the interface method they measure (e.g. "GetTargetGraph", "Compute").
+// Operation (op) names live in each consuming package's metrics.go, snake_cased
+// after the interface method they measure (e.g. "get_target_graph", "compute").
 
 // Tag keys.
 const (

orchestrator/native_orchestrator.go

@@ -208,7 +208,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT
 				GitClient:          gitModule,
 				Config:             repoCfg,
 				ExtraExcludedFiles: req.ExcludeFilesRegex,
-				Scope:              b.scope,
+				Scope:              b.scope.Tagged(map[string]string{metrics.TagRepo: url.ToShortRemote(build.Remote)}),
 			})
 		default:
 			return nil, tangoerrors.NewUser(fmt.Errorf("unknown computation strategy: %d", build.Strategy))