refactor(metrics): move graph cache lookup to orchestrator

controller/getchangedtargets.go

@@ -152,7 +152,7 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metric
 		return false, nil
 	}
 	if cachedReader == nil {
-		recordCacheLookup(e, opGetChangedTargets, _metricComparedTargetsCacheLookup, cacheErr)
+		metrics.RecordCacheLookup(e, opGetChangedTargets, metrics.ComparedTargetsCacheLookup, cacheErr)
 		return false, nil
 	}
 
@@ -190,7 +190,7 @@ func (c *controller) serveChangedTargetsFromCache(ctx context.Context, e *metric
 	logger.Info("GetChangedTargets: Cache hit, streaming from storage",
 		zap.Duration("cache_read_duration", cacheReadDuration),
 	)
-	recordCacheLookup(e, opGetChangedTargets, _metricComparedTargetsCacheLookup, nil)
+	metrics.RecordCacheLookup(e, opGetChangedTargets, metrics.ComparedTargetsCacheLookup, nil)
 	e.DurationHistogram(opGetChangedTargets, "cache_read_duration", metrics.FastDurationBuckets).RecordDuration(cacheReadDuration)
 	if sendErr := sendTrimmedChangedTargets(stream, cached, maxDist, request.GetOutputConfig()); sendErr != nil {
 		return false, fmt.Errorf("send cached response: %w", sendErr)
@@ -784,7 +784,7 @@ func readTreehash(ctx context.Context, st storage.Storage, buildDescription *pb.
 	}
 	key := cachekey.GetTreehashCachePath(entityBuild)
 	resp, err := st.Get(ctx, storage.DownloadRequest{Key: key})
-	recordCacheLookup(e, op, _metricTreehashCacheLookup, err)
+	metrics.RecordCacheLookup(e, op, metrics.TreehashCacheLookup, err)
 	if err != nil {
 		if storage.IsNotFound(err) {
 			return "", nil

controller/gettargetgraph.go

@@ -103,7 +103,7 @@ func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entit
 		// Look up the the git treehash based on cache path
 		treehashCachePath := cachekey.GetTreehashCachePath(req.Build)
 		treehashResponse, err := c.storage.Get(ctx, storage.DownloadRequest{Key: treehashCachePath})
-		recordCacheLookup(e, opGetTargetGraph, _metricTreehashCacheLookup, err)
+		metrics.RecordCacheLookup(e, opGetTargetGraph, metrics.TreehashCacheLookup, err)
 		if err != nil {
 			if storage.IsNotFound(err) {
 				// Cache miss - blob doesn't exist, need to compute and store target graph
@@ -126,7 +126,6 @@ func (c *controller) getGraph(ctx context.Context, e *metrics.Emitter, req entit
 			if ctx.Err() != nil {
 				err = ctx.Err()
 			}
-			recordCacheLookup(e, opGetTargetGraph, _metricGraphCacheLookup, err)
 			if err != nil {
 				if !storage.IsNotFound(err) {
 					return nil, fmt.Errorf("graph reader: %w", err)

controller/metrics.go

@@ -14,40 +14,9 @@
 
 package controller
 
-import (
-	"github.com/uber/tango/core/storage"
-	"github.com/uber/tango/observability/metrics"
-)
-
 // Operation names, snake_cased after the RPC interface methods they measure.
 const (
 	opGetTargetGraph        = "get_target_graph"
 	opGetChangedTargets     = "get_changed_targets"
 	opGetChangedTargetGraph = "get_changed_target_graph"
 )
-
-// Cache-lookup metric names. Each is emitted under its parent RPC op with a
-// result=hit|miss tag, so a hit rate is derivable per cache layer.
-const (
-	_metricTreehashCacheLookup        = "treehash_cache_lookup"
-	_metricGraphCacheLookup           = "graph_cache_lookup"
-	_metricComparedTargetsCacheLookup = "compared_targets_cache_lookup"
-)
-
-// recordCacheLookup emits a result-tagged counter for a cache lookup under the
-// given parent op: a nil error is a hit and a not-found error is a miss. Any
-// other error is an infra failure (already tracked by the failure metric), so
-// nothing is emitted — an infra error is not a cache miss and must not skew the
-// hit rate.
-func recordCacheLookup(e *metrics.Emitter, parentOp, name string, err error) {
-	var result string
-	switch {
-	case err == nil:
-		result = metrics.ResultHit
-	case storage.IsNotFound(err):
-		result = metrics.ResultMiss
-	default:
-		return
-	}
-	e.Tagged(map[string]string{metrics.TagResult: result}).Counter(parentOp, name).Inc(1)
-}

observability/metrics/BUILD.bazel

@@ -11,6 +11,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//core/errors",
+        "//core/storage",
         "@com_github_uber_go_tally//:tally",
     ],
 )

observability/metrics/names.go

@@ -16,6 +16,7 @@ package metrics
 
 import (
 	tangoerrors "github.com/uber/tango/core/errors"
+	"github.com/uber/tango/core/storage"
 )
 
 // Operation (op) names live in each consuming package's metrics.go, named after
@@ -34,6 +35,32 @@ const (
 	ResultMiss    = "miss"
 )
 
+// Cache-lookup metric names. Each is emitted under its parent RPC op with a
+// result=hit|miss tag, so a hit rate is derivable per cache layer.
+const (
+	TreehashCacheLookup        = "treehash_cache_lookup"
+	GraphCacheLookup           = "graph_cache_lookup"
+	ComparedTargetsCacheLookup = "compared_targets_cache_lookup"
+)
+
+// RecordCacheLookup emits a result-tagged counter for a cache lookup under the
+// given parent op: a nil error is a hit and a not-found error is a miss. Any
+// other error is an infra failure (already tracked by the failure metric), so
+// nothing is emitted — an infra error is not a cache miss and must not skew the
+// hit rate.
+func RecordCacheLookup(e *Emitter, parentOp, name string, err error) {
+	var result string
+	switch {
+	case err == nil:
+		result = ResultHit
+	case storage.IsNotFound(err):
+		result = ResultMiss
+	default:
+		return
+	}
+	e.Tagged(map[string]string{TagResult: result}).Counter(parentOp, name).Inc(1)
+}
+
 // Outcome maps an error to a result tag value. A nil error is "success";
 // any non-nil error delegates to tangoerrors.GetErrorCode, which returns
 // "cancelled", "user", "infra", or "infra_retryable".

orchestrator/native_orchestrator.go

@@ -173,6 +173,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT
 		cacheReadStart := time.Now()
 		graphReader, err := storage.NewGraphReader(ctx, b.storage, treehashPath)
 		recordStep(e, "cache_read_duration", cacheReadStart, metrics.FastDurationBuckets)
+		metrics.RecordCacheLookup(e, _opGetTargetGraph, metrics.GraphCacheLookup, err)
 		if err == nil {
 			logger.Infow("GetTargetGraph: Cache hit on treehash", zap.String("treehash", treehash))
 			return graphReader, nil