refactor(orchestrator): route metrics through observability/metrics Emitter#216
Merged
Conversation
xytan0056
force-pushed
the
pr3-orchestrator-metrics
branch
3 times, most recently
from
July 17, 2026 21:47
3b9c7e9 to
97b3839
Compare
xytan0056
force-pushed
the
pr2-controller-metrics
branch
from
July 17, 2026 23:10
d62bb17 to
7a8d276
Compare
xytan0056
force-pushed
the
pr3-orchestrator-metrics
branch
from
July 17, 2026 23:10
97b3839 to
57d3c09
Compare
xytan0056
changed the title
xytan0056
force-pushed
the
pr2-controller-metrics
branch
from
July 21, 2026 06:30
7a8d276 to
e77a1cb
Compare
xytan0056
commented
Jul 21, 2026
| return nil, fmt.Errorf("no repository configuration found for remote %q", remote) | ||
| } | ||
| leaseStart := time.Now() | ||
| ws, err := b.repoManager.Lease(ctx, build) |
Contributor
Author
There was a problem hiding this comment.
TODO: emit metrics before Lease error
yushan8
reviewed
Jul 21, 2026
| scope = tally.NoopScope | ||
| } | ||
| scope = scope.SubScope("orchestrator") | ||
| emitter, _ := metrics.New(scope) |
Contributor
There was a problem hiding this comment.
Should we fallback on metrics.Nop()? Seems like we're always silently discarding the error. I think we should either return the error or fallback to Nop().
yushan8
reviewed
Jul 21, 2026
| scope.Counter("success").Inc(1) | ||
| } | ||
| }() | ||
| op := metrics.Begin(b.emitter, opGetTargetGraph, stepDurationBuckets) |
Contributor
There was a problem hiding this comment.
Should we have the repo tagged in the emitter as well?
Contributor
Author
There was a problem hiding this comment.
should already be tagged in controller. but good piont, just in case NewNativeOrchestrator is called separately
yushan8
reviewed
Jul 21, 2026
| ) | ||
|
|
||
| // opGetTargetGraph is snake_cased after the Orchestrator.GetTargetGraph method. | ||
| const opGetTargetGraph = "get_target_graph" |
Contributor
There was a problem hiding this comment.
Suggested change
| const opGetTargetGraph = "get_target_graph" | |
| const _opGetTargetGraph = "get_target_graph" |
| // stepDurationBuckets covers whole-operation and individual pipeline steps | ||
| // (lease, checkout, apply, cache read/write, compute). A compute on a large | ||
| // repo can run for hours, so the range extends to ~4h: exponential 1ms..~4h. | ||
| var stepDurationBuckets = tally.MustMakeExponentialDurationBuckets(time.Millisecond, 3, 16) |
Contributor
There was a problem hiding this comment.
Suggested change
| var stepDurationBuckets = tally.MustMakeExponentialDurationBuckets(time.Millisecond, 3, 16) | |
| var _stepDurationBuckets = tally.MustMakeExponentialDurationBuckets(time.Millisecond, 3, 16) |
yushan8
approved these changes
Jul 21, 2026
xytan0056
force-pushed
the
pr2-controller-metrics
branch
from
July 21, 2026 20:26
e77a1cb to
2257789
Compare
xytan0056
force-pushed
the
pr3-orchestrator-metrics
branch
from
July 21, 2026 20:51
57d3c09 to
a9b63ab
Compare
xytan0056
force-pushed
the
pr3-orchestrator-metrics
branch
from
July 21, 2026 20:59
a9b63ab to
56b4726
Compare
…mitter Port native_orchestrator's GetTargetGraph metrics off raw tally.Scope to the Begin/Complete lifecycle helper. - Params keeps Scope tally.Scope; the orchestrator self-subscopes to "orchestrator" and builds its own emitter (still forwards the scope to the graph runner). - start + result-tagged finish (replaces calls/success/failure); recordFailure keeps the failure_type/failure_reason axis as a "failures" counter. - Buckets are package-level in orchestrator/metrics.go.
xytan0056
force-pushed
the
pr3-orchestrator-metrics
branch
from
July 21, 2026 22:00
56b4726 to
9a82429
Compare
xytan0056
added a commit
that referenced
this pull request
Jul 21, 2026
…itter (#217) Ports the native graph runner off raw `tally.Scope` to `*metrics.Emitter`. - `Params` keeps `Scope tally.Scope`; the runner builds its own emitter and emits under op `graph_runner` (nested beneath the orchestrator scope it's handed). - Three phase `Timer`s → `DurationHistogram` (`bazel_query_duration`, `git_file_hashes_duration`, `target_hash_duration`); `target_count` `Gauge` → `ValueHistogram`. - Buckets package-level in `graphrunner/metrics.go`. `core/bazel` intentionally left uninstrumented — the query is already timed here. 1. #216.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports
native_orchestrator'sGetTargetGraphoff rawtally.Scopeto*metrics.Emitterwith theBegin/Completehelper.ParamskeepsScope tally.Scope; the orchestrator self-subscopes toorchestratorand builds its own emitter (still forwards the scope to the graph runner).start+ result-taggedfinish(replacescalls/success/failure).get_target_graph:lease_duration,checkout_duration,apply_requests_duration,cache_read_duration,compute_duration,cache_write_duration.Stacked on #215.