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")