diff --git a/CLAUDE.md b/CLAUDE.md index 8a72232b..8f8249f6 100644 --- a/CLAUDE.md +++ b/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 diff --git a/controller/errors.go b/controller/errors.go index d5a7ab2d..45652188 100644 --- a/controller/errors.go +++ b/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) -} diff --git a/controller/getchangedtargetgraph.go b/controller/getchangedtargetgraph.go index df7bc77a..606abb4d 100644 --- a/controller/getchangedtargetgraph.go +++ b/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 } diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index 4f19fa04..6bafdd11 100644 --- a/controller/getchangedtargets.go +++ b/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) } }() diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index 2a2634e1..7976fb54 100644 --- a/controller/gettargetgraph.go +++ b/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) diff --git a/controller/metrics_test.go b/controller/metrics_test.go index 9da5fbb5..c43ba984 100644 --- a/controller/metrics_test.go +++ b/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") } diff --git a/docs/observability/metrics.md b/docs/observability/metrics.md index 7b935164..9d039693 100644 --- a/docs/observability/metrics.md +++ b/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`). - diff --git a/graphrunner/native.go b/graphrunner/native.go index 0c84b4d1..d47ded6f 100644 --- a/graphrunner/native.go +++ b/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)" diff --git a/observability/metrics/names.go b/observability/metrics/names.go index 48e1ad3c..b1469e47 100644 --- a/observability/metrics/names.go +++ b/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 ( diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index 4c0f7b41..79f6cc6d 100644 --- a/orchestrator/native_orchestrator.go +++ b/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))