feat(controller): emit result-tagged cache-lookup counters#241
Conversation
81a02eb to
95364c0
Compare
| // Download the target graph based on treehash. | ||
| storageStart := time.Now() | ||
| graphReader, err := storage.NewGraphReader(ctx, c.storage, treehashPath) | ||
| if err != nil && ctx.Err() != nil { |
There was a problem hiding this comment.
i dont think you need to check err != nil. No matter what if ctx.Err() I= nil we want to set err = ctx.Err()
| // recordCacheLookup emits a result-tagged counter for a cache lookup under the | ||
| // given parent op: hit when the payload was usable, miss otherwise (not-found | ||
| // or a storage failure both mean the payload must be recomputed). | ||
| func recordCacheLookup(e *metrics.Emitter, parentOp, name string, hit bool) { |
There was a problem hiding this comment.
can this take in err instead of hit so you can check if err == nil here instead of doing it at all the call sites?
There was a problem hiding this comment.
I think we only care if it's a notfounderror? Any other error I don't think we should emit this metric because it's an infra failure then?
There was a problem hiding this comment.
check if err == nil here
that's what I did initially--passing the error in, but there are cases when there's no error to pass like L196
recordCacheLookup(e, opGetChangedTargets, metricComparedTargetsCacheLookup, true)
I can pass in the error itself here
| if err != nil && ctx.Err() != nil { | ||
| err = ctx.Err() | ||
| } | ||
| recordCacheLookup(e, opGetTargetGraph, metricGraphCacheLookup, err == nil) |
There was a problem hiding this comment.
nit: Maybe we should do a check for if err == nil { cacheHit = true }
And then pass that into recordCacheLookup(e, opGetTargetGraph, metricGraphCacheLookup, cacheHit) for readability purposes. It's hard to tell why err == nil is passed into the function param of hit.
Also I think we should only emit this metric if storage.IsNotFound(err) in line 134 and line 136 separately. If it's neither a NotFoundError or err == nil, should it even emit a cache hit/miss metric?
There was a problem hiding this comment.
make sense. originally I was thinking all errors should be cache miss, but now i'm thinking cache hit should be a effectiveness meassurement not an outcome. The cachehit metrics should be honest here.
The only downside is hits + misses ≠ total attempts
e267415 to
977eef4
Compare
Replace the hit-only `cache_hit` counters with `result`-tagged lookup counters at every controller cache-lookup site, so a per-layer hit rate (hit / (hit + miss)) is derivable for each cache. Three counters are emitted under their parent RPC op: - treehash_cache_lookup (get_target_graph, get_changed_targets) - graph_cache_lookup (get_target_graph) - compared_targets_cache_lookup (get_changed_targets) recordCacheLookup classifies the lookup error: a nil error is a hit, a not-found error is a miss, and any other error is an infra failure that emits nothing (it is already tracked by the failure metric and must not skew the hit rate). In get_changed_targets the treehash lookup emits once per revision (from inside readTreehash); the write-path treehash reads that only build the cache key pass a no-op emitter so they do not skew the hit rate. BREAKING CHANGE: the bare `controller.get_target_graph.cache_hit` and `controller.get_changed_targets.cache_hit` counters are removed. Compute hit rate from the new result-tagged counters instead.
977eef4 to
4eaa916
Compare
3 participants
Replace the hit-only
cache_hitcounters withresult-tagged cache-lookup counters at every controller cache-lookup site, so a per-layer hit rate (hit / (hit + miss)) is derivable for each cache.Three counters, each emitted under its parent RPC op with a
result=hit|misstag:treehash_cache_lookupget_target_graph,get_changed_targetsgraph_cache_lookupget_target_graphcompared_targets_cache_lookupget_changed_targetsClassification
The result is classified by the lookup error's origin: a not-found is a genuine miss; an infra failure is not a cache miss (it's already tracked by the failure metric) and is left out so it can't skew the hit rate.
Breaking: the bare
controller.get_target_graph.cache_hitandcontroller.get_changed_targets.cache_hitcounters are removed.Stacked on
refactor/share-metrics-buckets(#240) — review/merge that first.