Repository navigation
refactor(handler): move the metrics lifecycle to the handler - #333
Merged
Merged
Conversation
The handler now owns the metrics lifecycle of every RPC, including the stub GetChangedTargetGraph. For each RPC the handler maps the proto request and wraps a mapping error as a user error. It resolves the repository with its own resolveRequestRepository, which uses handler.Params.RepoConfig. It starts the metrics.Op, tags it with the repository ID or with "unknown", and completes it in the same deferred block as the failure log and the wire conversion. The failure log has a repository field. A request that fails the mapping or the repository check never reaches the controller. The handler then calls the controller with the original proto request and stream. The controller no longer records start and finish. It still maps the request and resolves the repository for its cache keys and seed attributes. It keeps its phase metrics and its send_duration under the "controller" scope. The stub GetChangedTargetGraph stays in the controller and no longer records metrics. The handler calls it inside the lifecycle. handler.Params gets RepoConfig and Scope. The handler and the controller both map the request and resolve the repository. The duplicate code carries a comment that names it as temporary. The lifecycle metrics start and finish publish under handler.<operation> instead of controller.<operation>. The names, tags, and counts do not change. Dashboards and alerts that read controller.get_target_graph.start, controller.get_target_graph.finish, controller.get_changed_targets.start, controller.get_changed_targets.finish, controller.get_changed_target_graph.start, or controller.get_changed_target_graph.finish must read the handler path. The metrics guide describes this. Test plan: - make build, make test, make lint - handler tests cover the lifecycle metrics of each RPC with the repo tag - a comparison of the metric names, tags, and result tags of a real handler and controller against origin/main for valid, invalid, unconfigured, and stub requests showed no difference except the lifecycle prefix Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
yushan8
approved these changes
Oct 15, 2026
justinwon777
added a commit
that referenced
this pull request
Oct 15, 2026
… repository (#334) **Stack** (each PR is based on the one before it): 1. #333 Move the metrics lifecycle to the handler 2. #334 `GetTargetGraph` takes the entity request and the repository (this PR) 3. #335 `GetChangedTargets` takes the entity request and the repository 4. #336 Remove the repository resolver Depends on #333. Review and merge in order. ## What The handler passes the entity request and the resolved `config.RepositoryConfig` to `GetTargetGraph` in the controller. The controller no longer maps the request or resolves the repository for this RPC. It uses the repository ID for cache keys and phase metrics. It still filters and sends to the stream. The `Controller` interface no longer embeds the generated server, because its first method now takes entity types. It lists `GetTargetGraph` and `GetChangedTargets`. The stub `GetChangedTargetGraph` leaves the controller. The handler serves it: it starts the metrics op, returns the Unimplemented infra error, and completes the op. It makes no controller call. The controller keeps its resolver for `GetChangedTargets` only. ## Test plan - make build, make test, make lint - Handler tests check the forwarded entity request, output config, stream, and repository. - Controller tests call `GetTargetGraph` with the entity request. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
justinwon777
added a commit
that referenced
this pull request
Oct 15, 2026
…the repository (#335) **Stack** (each PR is based on the one before it): 1. #333 Move the metrics lifecycle to the handler 2. #334 `GetTargetGraph` takes the entity request and the repository 3. #335 `GetChangedTargets` takes the entity request and the repository (this PR) 4. #336 Remove the repository resolver Depends on #334. Review and merge in order. ## What The handler passes the entity request and the resolved `config.RepositoryConfig` to `GetChangedTargets` in the controller. The controller no longer maps the request or resolves the repository for this RPC. It uses the repository ID for cache keys and phase metrics, and the repository configuration for seed attributes. It still filters and sends to the stream. The controller test `TestGetChangedTargets_ValidationError` goes. It passed a nil proto request, so it tested the conversion that the handler owns. The mapper tests cover that conversion. The controller keeps its resolver, `controller.Params.RepoConfig`, and their test in this PR. The next PR removes them. ## Test plan - make build, make test, make lint - Handler tests check the forwarded entity request, output config, stream, and repository. - Controller tests build the entity request and the repository at each call site and call `GetChangedTargets` directly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
justinwon777
added a commit
that referenced
this pull request
Oct 15, 2026
**Stack** (each PR is based on the one before it): 1. #333 Move the metrics lifecycle to the handler 2. #334 `GetTargetGraph` takes the entity request and the repository 3. #335 `GetChangedTargets` takes the entity request and the repository 4. #336 Remove the repository resolver (this PR) Depends on #335. Review and merge in order. ## What The controller no longer uses its repository resolver, because both RPCs take the resolved repository from the handler. This PR removes the dead code: - `resolveRequestRepository`, `unknownRepositoryMetricLabel`, `controller.Params.RepoConfig`, and the `repoConfig` field of the controller - the controller test of the resolver (the handler has its own test of the same function) - the `RepoConfig` setting on `controller.Params` in tests, `example`, and the integration test - the handler comments that named its copies as temporary ## Breaking change for callers Callers that build `controller.Params`, such as go-code, must stop setting `RepoConfig`. They must set it on `handler.Params` instead. ## Test plan - make build, make test, make lint 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
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.
Stack (each PR is based on the one before it):
GetTargetGraphtakes the entity request and the repositoryGetChangedTargetstakes the entity request and the repositoryWhat
unknownand never reaches the controller.target_count) andsend_duration.startandfinishnow publish underhandler.<operation>instead ofcontroller.<operation>. Names, tags, and counts do not change.Temporary duplication
The handler and the controller both map the request and resolve the repository. The controller needs the result for cache keys. A follow-up change passes the entity request and the resolved repository to the controller and removes the duplicate. The duplicate code has comments that say so.
Test plan:
Issue
#325