From eacf19300d22c45a97fc945ed5a5a1078ec3f94f Mon Sep 17 00:00:00 2001 From: Justin Won Date: Wed, 14 Oct 2026 18:57:39 -0700 Subject: [PATCH] refactor(handler): move the metrics lifecycle to the handler 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. instead of controller.. 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 --- AGENTS.md | 2 +- controller/BUILD.bazel | 1 - controller/README.md | 13 +++--- controller/controller.go | 5 +++ controller/getchangedtargetgraph.go | 10 +---- controller/getchangedtargets.go | 6 +-- controller/gettargetgraph.go | 6 +-- controller/metrics.go | 5 +-- controller/metrics_test.go | 43 ------------------ docs/observability/metrics.md | 8 ++-- example/main.go | 2 +- handler/BUILD.bazel | 10 +++++ handler/getchangedtargetgraph.go | 7 ++- handler/getchangedtargets.go | 21 ++++++++- handler/getchangedtargets_test.go | 7 ++- handler/gettargetgraph.go | 21 ++++++++- handler/gettargetgraph_test.go | 4 +- handler/handler.go | 43 +++++++++++++++++- handler/handler_test.go | 48 ++++++++++++++++++++ handler/metrics.go | 23 ++++++++++ handler/metrics_test.go | 70 +++++++++++++++++++++++++++++ handler/wireerror_test.go | 7 ++- integration/integration_test.go | 2 +- 23 files changed, 273 insertions(+), 91 deletions(-) delete mode 100644 controller/metrics_test.go create mode 100644 handler/handler_test.go create mode 100644 handler/metrics.go create mode 100644 handler/metrics_test.go diff --git a/AGENTS.md b/AGENTS.md index 104c67c7..098f8634 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,7 +65,7 @@ The top-level split is by **responsibility**, not by domain: `controller/` handl The controller is the YARPC service implementation. It owns transport-adjacent concerns: request validation, metrics, cancellation linkage, response streaming, output filtering, comparison fan-out, and compared-target caching. It does **not** own workspace creation, git operations, or graph computation. -Every RPC records operation metrics, classifies invalid input as a user error, preserves error chains, attempts applicable cache reads before expensive work, delegates graph production to the orchestrator, and converts the final error at the wire boundary. +The handler records the operation metrics of every RPC, under the `handler` scope. Every RPC classifies invalid input as a user error, preserves error chains, attempts applicable cache reads before expensive work, delegates graph production to the orchestrator, and converts the final error at the wire boundary. ### Orchestrator diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 0f3c0fee..838a800d 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -44,7 +44,6 @@ go_test( "getchangedtargets_test.go", "getchangedtargets_tgb_test.go", "gettargetgraph_test.go", - "metrics_test.go", "output_filter_test.go", "testhelper_test.go", ], diff --git a/controller/README.md b/controller/README.md index 9da7c33a..30277f29 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1,7 +1,7 @@ # Controller -The controller is the request-handling layer of Tango. It implements the -generated YARPC service surface and turns each streaming RPC into a +The controller is the request-handling layer of Tango. The handler calls +it, and it turns each streaming RPC into a deterministic pipeline of cache lookup, graph computation, comparison, and response streaming. @@ -24,9 +24,9 @@ sit between the wire protocol and the rest of the system: - **Streaming and chunking.** Responses are emitted as multiple stream messages sized to stay below the gRPC per-message limit. Targets, metadata, and topology deltas are chunked independently. -- **Observability.** Every RPC emits per-call counters, per-phase timers, - and a classified failure metric that distinguishes user from - infrastructure errors. +- **Observability.** Every RPC emits per-phase timers. The handler emits the + per-call counters and the classified failure metric that distinguishes + user from infrastructure errors. ## Collaborators @@ -46,5 +46,4 @@ behavior: The controller is built once at startup with its logger, storage, orchestrator, optional metrics scope, and optional max-message-bytes configuration. The constructor returns the -generated server interface, so the controller can be registered with a -YARPC dispatcher without additional adaptation. +`Controller` interface, which the handler calls. diff --git a/controller/controller.go b/controller/controller.go index c8ed59ad..a74de488 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -30,6 +30,8 @@ import ( "go.uber.org/zap" ) +// Duplicated in the handler. Temporary: the controller drops this copy when it +// takes the resolved repository from the handler. const unknownRepositoryMetricLabel = "unknown" // Params are the parameters for the controller. @@ -48,6 +50,9 @@ type Params struct { // for a validated request. Invalid requests retain the common unknown label and // their existing error; valid requests must exactly match the configured // repository allowlist before controller cache I/O. +// +// Duplicated in the handler. Temporary: the controller drops this copy when it +// takes the resolved repository from the handler. func (c *controller) resolveRequestRepository(remote string, requestErr error) (config.RepositoryConfig, string, error) { if requestErr != nil { return config.RepositoryConfig{}, unknownRepositoryMetricLabel, requestErr diff --git a/controller/getchangedtargetgraph.go b/controller/getchangedtargetgraph.go index 606abb4d..e7eb6575 100644 --- a/controller/getchangedtargetgraph.go +++ b/controller/getchangedtargetgraph.go @@ -16,7 +16,6 @@ package controller import ( tangoerrors "github.com/uber/tango/core/errors" - "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" "go.uber.org/yarpc/yarpcerrors" ) @@ -25,13 +24,8 @@ import ( // induced by the changed targets between two revisions. NOT YET IMPLEMENTED: // returns a YARPC Unimplemented error so callers get an explicit failure // instead of a silent empty stream. -func (c *controller) GetChangedTargetGraph(request *pb.GetChangedTargetGraphRequest, stream pb.TangoServiceGetChangedTargetGraphYARPCServer) (retErr error) { - op := metrics.Begin(c.emitter, opGetChangedTargetGraph, metrics.SlowDurationBuckets) - retErr = tangoerrors.NewInfra( +func (c *controller) GetChangedTargetGraph(request *pb.GetChangedTargetGraphRequest, stream pb.TangoServiceGetChangedTargetGraphYARPCServer) error { + return tangoerrors.NewInfra( yarpcerrors.Newf(yarpcerrors.CodeUnimplemented, "GetChangedTargetGraph is not yet implemented"), ) - defer func() { - op.Complete(retErr) - }() - return retErr } diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index 941565eb..c86a1d58 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -82,20 +82,16 @@ type job struct { // GetChangedTargets returns the changed targets between two revisions. If the // client disconnects, the stream's context is cancelled and the function // returns with context.Canceled. -func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) { +func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) error { entityReq, mappingErr := proto.ProtoToGetChangedTargetsRequest(request) if mappingErr != nil { mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get changed targets request: %w", mappingErr)) } repoCfg, repo, repositoryErr := c.resolveRequestRepository(entityReq.First.Remote, mappingErr) e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) - op := metrics.Begin(e, opGetChangedTargets, metrics.SlowDurationBuckets) logger := c.logger.WithLazy( zap.String("repository", repo), ) - defer func() { - op.Complete(retErr) - }() if repositoryErr != nil { return repositoryErr } diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index 621fd95f..b6b23c05 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -34,20 +34,16 @@ import ( ) // GetTargetGraph returns the target graph for a given request. -func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) { +func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) error { entityReq, mappingErr := mapper.ProtoToGetTargetGraphRequest(request) if mappingErr != nil { mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", mappingErr)) } repoCfg, repo, repositoryErr := c.resolveRequestRepository(entityReq.Build.Remote, mappingErr) e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) - op := metrics.Begin(e, opGetTargetGraph, metrics.SlowDurationBuckets) logger := c.logger.WithLazy( zap.String("repository", repo), ) - defer func() { - op.Complete(retErr) - }() start := time.Now() ctx, cancelLink := c.linkRequestCtx(stream.Context()) defer cancelLink() diff --git a/controller/metrics.go b/controller/metrics.go index 559e8ee3..d4aafeda 100644 --- a/controller/metrics.go +++ b/controller/metrics.go @@ -16,7 +16,6 @@ package controller // 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" + opGetTargetGraph = "get_target_graph" + opGetChangedTargets = "get_changed_targets" ) diff --git a/controller/metrics_test.go b/controller/metrics_test.go deleted file mode 100644 index c43ba984..00000000 --- a/controller/metrics_test.go +++ /dev/null @@ -1,43 +0,0 @@ -// Copyright (c) 2026 Uber Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package controller - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/uber-go/tally" - "github.com/uber/tango/observability/metrics" - "go.uber.org/zap" -) - -// TestControllerMetricsPathShape drives the stub GetChangedTargetGraph handler -// through a TestScope-backed emitter and asserts the start/finish lifecycle -// lands under .. with the outcome tag. -// The handler returns Unimplemented (classified as an infrastructure error), -// so the finish histogram carries result=infra rather than result=success. -func TestControllerMetricsPathShape(t *testing.T) { - ts := tally.NewTestScope("controller", nil) - e := metrics.New(ts) - - c := &controller{logger: zap.NewNop(), emitter: e, appCtx: context.Background()} - require.Error(t, c.GetChangedTargetGraph(nil, nil)) - - 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") -} diff --git a/docs/observability/metrics.md b/docs/observability/metrics.md index f071ec2d..614575cb 100644 --- a/docs/observability/metrics.md +++ b/docs/observability/metrics.md @@ -208,16 +208,16 @@ if err == nil { ``` # operation rate -fetch service:tango name:controller.get_changed_targets.start +fetch service:tango name:handler.get_changed_targets.start # success and classified error counts -fetch service:tango name:controller.get_changed_targets.finish | sum by (result) +fetch service:tango name:handler.get_changed_targets.finish | sum by (result) # P95 latency of successful requests -fetch service:tango name:controller.get_changed_targets.finish result:success | histogram_percentile(95) +fetch service:tango name:handler.get_changed_targets.finish result:success | histogram_percentile(95) # scoped to a repo -fetch service:tango name:controller.get_changed_targets.finish result:success repo:my-monorepo | histogram_percentile(95) +fetch service:tango name:handler.get_changed_targets.finish result:success repo:my-monorepo | histogram_percentile(95) # custom value metric — changed-target count distribution fetch service:tango name:controller.get_changed_targets.target_count | histogram_percentile(95) diff --git a/example/main.go b/example/main.go index 449a7b1a..e3977c0e 100644 --- a/example/main.go +++ b/example/main.go @@ -109,7 +109,7 @@ func run() error { GraphConfig: cfg, }) - h := handler.New(handler.Params{Logger: logger, Controller: ctrl}) + h := handler.New(handler.Params{Logger: logger, Controller: ctrl, RepoConfig: cfg}) // YARPC transports and dispatcher grpcTransport := yarpcgrpc.NewTransport() diff --git a/handler/BUILD.bazel b/handler/BUILD.bazel index e049736e..e0e62bda 100644 --- a/handler/BUILD.bazel +++ b/handler/BUILD.bazel @@ -8,14 +8,20 @@ go_library( "getchangedtargets.go", "gettargetgraph.go", "handler.go", + "metrics.go", ], importpath = "github.com/uber/tango/handler", visibility = ["//visibility:public"], deps = [ + "//config", "//controller", "//core/errors", + "//internal/mapper", "//mapper/proto", + "//observability/metrics", "//tangopb", + "@com_github_uber_go_tally//:tally", + "@org_uber_go_fx//:fx", "@org_uber_go_zap//:zap", ], ) @@ -26,6 +32,8 @@ go_test( "getchangedtargetgraph_test.go", "getchangedtargets_test.go", "gettargetgraph_test.go", + "handler_test.go", + "metrics_test.go", "testhelper_test.go", "wireerror_test.go", ], @@ -41,9 +49,11 @@ go_test( "//tangopb/tangopbmock", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@com_github_uber_go_tally//:tally", "@org_uber_go_mock//gomock", "@org_uber_go_yarpc//encoding/protobuf", "@org_uber_go_yarpc//yarpcerrors", + "@org_uber_go_zap//:zap", "@org_uber_go_zap//zaptest", ], ) diff --git a/handler/getchangedtargetgraph.go b/handler/getchangedtargetgraph.go index 24d912a5..9b06b0a6 100644 --- a/handler/getchangedtargetgraph.go +++ b/handler/getchangedtargetgraph.go @@ -15,9 +15,14 @@ package handler import ( + "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" ) -func (h *handler) GetChangedTargetGraph(request *pb.GetChangedTargetGraphRequest, stream pb.TangoServiceGetChangedTargetGraphYARPCServer) error { +func (h *handler) GetChangedTargetGraph(request *pb.GetChangedTargetGraphRequest, stream pb.TangoServiceGetChangedTargetGraphYARPCServer) (retErr error) { + op := metrics.Begin(h.emitter, opGetChangedTargetGraph, metrics.SlowDurationBuckets) + defer func() { + op.Complete(retErr) + }() return h.controller.GetChangedTargetGraph(request, stream) } diff --git a/handler/getchangedtargets.go b/handler/getchangedtargets.go index a7a064ad..dfb227fc 100644 --- a/handler/getchangedtargets.go +++ b/handler/getchangedtargets.go @@ -15,16 +15,35 @@ package handler import ( + "fmt" + tangoerrors "github.com/uber/tango/core/errors" + "github.com/uber/tango/mapper/proto" + "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" + "go.uber.org/zap" ) func (h *handler) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) { + // Temporary: the handler maps the request only to tag the metrics. The + // controller maps it again until it takes the entity request. + entityReq, mappingErr := proto.ProtoToGetChangedTargetsRequest(request) + if mappingErr != nil { + mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get changed targets request: %w", mappingErr)) + } + _, repo, repositoryErr := h.resolveRequestRepository(entityReq.First.Remote, mappingErr) + e := h.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) + op := metrics.Begin(e, opGetChangedTargets, metrics.SlowDurationBuckets) + logger := h.logger.WithLazy(zap.String("repository", repo)) defer func() { + op.Complete(retErr) if retErr != nil { - h.logger.Error("GetChangedTargets failed", tangoerrors.Fields(retErr)...) + logger.Error("GetChangedTargets failed", tangoerrors.Fields(retErr)...) retErr = toWireError(retErr) } }() + if repositoryErr != nil { + return repositoryErr + } return h.controller.GetChangedTargets(request, stream) } diff --git a/handler/getchangedtargets_test.go b/handler/getchangedtargets_test.go index f12b3811..0a6bdd2b 100644 --- a/handler/getchangedtargets_test.go +++ b/handler/getchangedtargets_test.go @@ -31,9 +31,12 @@ type fakeGetChangedTargetsStream struct { func TestGetChangedTargetsForwardsToController(t *testing.T) { ctrl := mock_controller.NewMockController(gomock.NewController(t)) - h := New(Params{Logger: zaptest.NewLogger(t), Controller: ctrl}) + h := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: ctrl}) - req := &pb.GetChangedTargetsRequest{} + req := &pb.GetChangedTargetsRequest{ + FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, + SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, + } stream := &fakeGetChangedTargetsStream{} ctrl.EXPECT().GetChangedTargets(gomock.Any(), gomock.Any()).DoAndReturn( func(gotReq *pb.GetChangedTargetsRequest, gotStream pb.TangoServiceGetChangedTargetsYARPCServer) error { diff --git a/handler/gettargetgraph.go b/handler/gettargetgraph.go index c0b1c4e4..7aa48b3f 100644 --- a/handler/gettargetgraph.go +++ b/handler/gettargetgraph.go @@ -15,16 +15,35 @@ package handler import ( + "fmt" + tangoerrors "github.com/uber/tango/core/errors" + "github.com/uber/tango/internal/mapper" + "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" + "go.uber.org/zap" ) func (h *handler) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) { + // Temporary: the handler maps the request only to tag the metrics. The + // controller maps it again until it takes the entity request. + entityReq, mappingErr := mapper.ProtoToGetTargetGraphRequest(request) + if mappingErr != nil { + mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", mappingErr)) + } + _, repo, repositoryErr := h.resolveRequestRepository(entityReq.Build.Remote, mappingErr) + e := h.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) + op := metrics.Begin(e, opGetTargetGraph, metrics.SlowDurationBuckets) + logger := h.logger.WithLazy(zap.String("repository", repo)) defer func() { + op.Complete(retErr) if retErr != nil { - h.logger.Error("GetTargetGraph failed", tangoerrors.Fields(retErr)...) + logger.Error("GetTargetGraph failed", tangoerrors.Fields(retErr)...) retErr = toWireError(retErr) } }() + if repositoryErr != nil { + return repositoryErr + } return h.controller.GetTargetGraph(request, stream) } diff --git a/handler/gettargetgraph_test.go b/handler/gettargetgraph_test.go index 58da7e74..0bb3e1b3 100644 --- a/handler/gettargetgraph_test.go +++ b/handler/gettargetgraph_test.go @@ -31,9 +31,9 @@ type fakeGetTargetGraphStream struct { func TestGetTargetGraphForwardsToController(t *testing.T) { ctrl := mock_controller.NewMockController(gomock.NewController(t)) - h := New(Params{Logger: zaptest.NewLogger(t), Controller: ctrl}) + h := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: ctrl}) - req := &pb.GetTargetGraphRequest{} + req := &pb.GetTargetGraphRequest{BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}} stream := &fakeGetTargetGraphStream{} ctrl.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).DoAndReturn( func(gotReq *pb.GetTargetGraphRequest, gotStream pb.TangoServiceGetTargetGraphYARPCServer) error { diff --git a/handler/handler.go b/handler/handler.go index 2c0f5396..a186cdc6 100644 --- a/handler/handler.go +++ b/handler/handler.go @@ -15,21 +15,62 @@ package handler import ( + "fmt" + + "github.com/uber-go/tally" + "github.com/uber/tango/config" "github.com/uber/tango/controller" + tangoerrors "github.com/uber/tango/core/errors" + "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" + "go.uber.org/fx" "go.uber.org/zap" ) +// Duplicate of the constant in the controller. Temporary: the controller +// drops its copy when it takes the resolved repository from the handler. +const unknownRepositoryMetricLabel = "unknown" + type Params struct { + fx.In Logger *zap.Logger Controller controller.Controller + RepoConfig config.RepositoryConfigProvider + Scope tally.Scope `optional:"true"` } type handler struct { logger *zap.Logger controller controller.Controller + repoConfig config.RepositoryConfigProvider + emitter *metrics.Emitter } func New(p Params) pb.TangoYARPCServer { - return &handler{logger: p.Logger, controller: p.Controller} + return &handler{ + logger: p.Logger, + controller: p.Controller, + repoConfig: p.RepoConfig, + emitter: metrics.New(p.Scope).SubScope("handler"), + } +} + +// resolveRequestRepository returns the configured repository and metric label +// for a validated request. Invalid requests retain the common unknown label and +// their existing error; valid requests must exactly match the configured +// repository allowlist before controller cache I/O. +// +// Duplicate of controller.resolveRequestRepository. Temporary: the controller +// drops its copy when it takes the resolved repository from the handler. +func (h *handler) resolveRequestRepository(remote string, requestErr error) (config.RepositoryConfig, string, error) { + if requestErr != nil { + return config.RepositoryConfig{}, unknownRepositoryMetricLabel, requestErr + } + repo, ok := h.repoConfig.GetRepositoryConfig(remote) + if !ok { + return config.RepositoryConfig{}, unknownRepositoryMetricLabel, tangoerrors.NewUser( + fmt.Errorf("repository remote %q is not configured", remote), + ) + } + return repo, repo.RepositoryID, nil } diff --git a/handler/handler_test.go b/handler/handler_test.go new file mode 100644 index 00000000..b31ebc62 --- /dev/null +++ b/handler/handler_test.go @@ -0,0 +1,48 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package handler + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber/tango/config" + tangoerrors "github.com/uber/tango/core/errors" +) + +type rejectAllRepositoryConfigProvider struct{} + +func (rejectAllRepositoryConfigProvider) GetRepositoryConfig(string) (config.RepositoryConfig, bool) { + return config.RepositoryConfig{}, false +} + +func TestResolveRequestRepository(t *testing.T) { + h := &handler{repoConfig: rejectAllRepositoryConfigProvider{}} + + t.Run("request error uses unknown repository", func(t *testing.T) { + repo, label, err := h.resolveRequestRepository("ignored", assert.AnError) + assert.Empty(t, repo) + assert.Equal(t, unknownRepositoryMetricLabel, label) + assert.ErrorIs(t, err, assert.AnError) + }) + + t.Run("plain remote", func(t *testing.T) { + _, label, err := h.resolveRequestRepository("git@github.com:other/repo.git", nil) + require.Error(t, err) + assert.Equal(t, unknownRepositoryMetricLabel, label) + assert.Equal(t, tangoerrors.ErrorUser, tangoerrors.GetErrorCode(err)) + }) +} diff --git a/handler/metrics.go b/handler/metrics.go new file mode 100644 index 00000000..9681e9fe --- /dev/null +++ b/handler/metrics.go @@ -0,0 +1,23 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package handler + +// Duplicates the operation names in the controller, which uses them for its +// phase metrics. +const ( + opGetTargetGraph = "get_target_graph" + opGetChangedTargets = "get_changed_targets" + opGetChangedTargetGraph = "get_changed_target_graph" +) diff --git a/handler/metrics_test.go b/handler/metrics_test.go new file mode 100644 index 00000000..d0e915de --- /dev/null +++ b/handler/metrics_test.go @@ -0,0 +1,70 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package handler + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + mock_controller "github.com/uber/tango/controller/controllermock" + tangoerrors "github.com/uber/tango/core/errors" + pb "github.com/uber/tango/tangopb" + "go.uber.org/mock/gomock" + "go.uber.org/zap" +) + +func TestGetTargetGraphMetrics(t *testing.T) { + ts := tally.NewTestScope("", nil) + ctrl := mock_controller.NewMockController(gomock.NewController(t)) + ctrl.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(nil) + h := New(Params{Logger: zap.NewNop(), Controller: ctrl, RepoConfig: allowAnyRepositoryConfigProvider{}, Scope: ts}) + + build := &pb.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", Strategy: pb.COMPUTATION_STRATEGY_UNSET} + require.NoError(t, h.GetTargetGraph(&pb.GetTargetGraphRequest{BuildDescription: build}, nil)) + + snap := ts.Snapshot() + assert.Contains(t, snap.Counters(), "handler.get_target_graph.start+repo=test-repository") + assert.Contains(t, snap.Histograms(), "handler.get_target_graph.finish+repo=test-repository,result=success") +} + +func TestGetChangedTargetsMetrics(t *testing.T) { + ts := tally.NewTestScope("", nil) + ctrl := mock_controller.NewMockController(gomock.NewController(t)) + ctrl.EXPECT().GetChangedTargets(gomock.Any(), gomock.Any()).Return(nil) + h := New(Params{Logger: zap.NewNop(), Controller: ctrl, RepoConfig: allowAnyRepositoryConfigProvider{}, Scope: ts}) + + build := &pb.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", Strategy: pb.COMPUTATION_STRATEGY_UNSET} + require.NoError(t, h.GetChangedTargets(&pb.GetChangedTargetsRequest{FirstRevision: build, SecondRevision: build}, nil)) + + snap := ts.Snapshot() + assert.Contains(t, snap.Counters(), "handler.get_changed_targets.start+repo=test-repository") + assert.Contains(t, snap.Histograms(), "handler.get_changed_targets.finish+repo=test-repository,result=success") +} + +func TestGetChangedTargetGraphMetrics(t *testing.T) { + ts := tally.NewTestScope("", nil) + ctrl := mock_controller.NewMockController(gomock.NewController(t)) + ctrl.EXPECT().GetChangedTargetGraph(gomock.Any(), gomock.Any()).Return(tangoerrors.NewInfra(errors.New("unimplemented"))) + h := New(Params{Logger: zap.NewNop(), Controller: ctrl, Scope: ts}) + + require.Error(t, h.GetChangedTargetGraph(nil, nil)) + + snap := ts.Snapshot() + assert.Contains(t, snap.Counters(), "handler.get_changed_target_graph.start+") + assert.Contains(t, snap.Histograms(), "handler.get_changed_target_graph.finish+result=infra") +} diff --git a/handler/wireerror_test.go b/handler/wireerror_test.go index 7569df55..eaf661c5 100644 --- a/handler/wireerror_test.go +++ b/handler/wireerror_test.go @@ -87,9 +87,8 @@ func TestHandlerErrors_ReturnTangoErrorDetail(t *testing.T) { func TestGetTargetGraph_ValidationError_WiresTangoError(t *testing.T) { ctrl := gomock.NewController(t) stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) - stream.EXPECT().Context().Return(context.Background()) - c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ + c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), })}) @@ -118,7 +117,7 @@ func TestGetChangedTargets_ValidationError_WiresTangoError(t *testing.T) { stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) stream.EXPECT().Context().Return(context.Background()).AnyTimes() - c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ + c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), })}) @@ -147,7 +146,7 @@ func TestGetTargetGraph_InfraError_WiresTangoError(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("disk on fire")) - c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ + c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, diff --git a/integration/integration_test.go b/integration/integration_test.go index 05afcf99..eaf805f7 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -156,7 +156,7 @@ func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { GraphConfig: cfg, }) - h := handler.New(handler.Params{Logger: zl, Controller: ctrl}) + h := handler.New(handler.Params{Logger: zl, Controller: ctrl, RepoConfig: cfg}) grpcTransport := yarpcgrpc.NewTransport() listener, err := net.Listen("tcp", "127.0.0.1:0")