From c9b139b49431952bd208b2b8039f97e0ba3d484b Mon Sep 17 00:00:00 2001 From: yushan Date: Fri, 5 Jun 2026 23:30:08 +0000 Subject: [PATCH 1/2] Add additional metrics for query duration and target hash --- controller/getchangedtargets.go | 2 +- controller/getchangedtargetsandedges.go | 2 +- controller/gettargetgraph.go | 2 +- core/targethasher/graph.go | 20 +++++++++++++------- orchestrator/native_orchestrator.go | 1 + 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index 28e8ccc..38c0a37 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -41,6 +41,7 @@ type job struct { // GetChangedTargets returns the changed targets between two revisions. func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) { scope := c.scope.SubScope("get_changed_targets") + scope.Counter("calls").Inc(1) defer func() { if retErr != nil { scope.Counter("failure").Inc(1) @@ -54,7 +55,6 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str return common.WithReason(failureReasonValidation, common.ErrorTypeUser, err) } scope = scope.Tagged(map[string]string{"repo": common.ToShortRemote(request.GetFirstRevision().GetRemote())}) - scope.Counter("calls").Inc(1) ctx := stream.Context() start := time.Now() logger := c.logger.With( diff --git a/controller/getchangedtargetsandedges.go b/controller/getchangedtargetsandedges.go index 015da21..774cc59 100644 --- a/controller/getchangedtargetsandedges.go +++ b/controller/getchangedtargetsandedges.go @@ -41,6 +41,7 @@ func packEdge(src, dep int32) uint64 { // GetChangedTargetsAndEdges returns the changed targets and edges between two revisions. func (c *controller) GetChangedTargetsAndEdges(request *pb.GetChangedTargetsAndEdgesRequest, stream pb.TangoServiceGetChangedTargetsAndEdgesYARPCServer) (retErr error) { scope := c.scope.SubScope("get_changed_targets_and_edges") + scope.Counter("calls").Inc(1) defer func() { if retErr != nil { scope.Counter("failure").Inc(1) @@ -54,7 +55,6 @@ func (c *controller) GetChangedTargetsAndEdges(request *pb.GetChangedTargetsAndE return common.WithReason(failureReasonValidation, common.ErrorTypeUser, err) } scope = scope.Tagged(map[string]string{"repo": common.ToShortRemote(request.GetFirstRevision().GetRemote())}) - scope.Counter("calls").Inc(1) ctx := stream.Context() start := time.Now() logger := c.logger.With( diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index a335cd9..b640969 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -32,6 +32,7 @@ import ( // GetTargetGraph returns the target graph for a given request. func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) { scope := c.scope.SubScope("get_target_graph") + scope.Counter("calls").Inc(1) defer func() { if retErr != nil { scope.Counter("failure").Inc(1) @@ -46,7 +47,6 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb zap.Any("build_description", request.GetBuildDescription()), ) scope = scope.Tagged(map[string]string{"repo": common.ToShortRemote(request.GetBuildDescription().GetRemote())}) - scope.Counter("calls").Inc(1) graphReader, err := c.getGraph(ctx, request.GetBuildDescription(), request.GetOutputConfig(), request.GetRequestOptions(), request.GetBypassCache()) if err != nil { return err diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 599eb66..29ad9a5 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -630,10 +630,10 @@ func HashRecursively(ctx context.Context, p HashParam) ([]byte, error) { return []byte{}, nil } - // Mark node as visited by setting it to an empty slice instead of nil slice which it got by default - to avoid - // cycles. It would be reset to a real hash value once dependencies are traversed. + // Mark node as visited by setting Hash to an empty slice (non-nil) to break cycles. + // HashWithoutDeps is intentionally left as-is: toTarget already computed it, and + // HashRecursively reuses it below to avoid calling HashRuleCommon twice per rule. target.Hash = []byte{} - target.HashWithoutDeps = []byte{} var hash []byte var hashWithoutDeps []byte @@ -663,11 +663,17 @@ func HashRecursively(ctx context.Context, p HashParam) ([]byte, error) { h.Write([]byte(p.TargetName)) hash = h.Sum(nil) default: - // Regular rule + // Regular rule: hash = sha1(ruleBody || dep1Hash || dep2Hash || ...) + // toTarget already called HashRuleCommon and stored the result in target.HashWithoutDeps, + // so reuse it here rather than re-running HashRuleCommon over all attributes again. h := newHash() - noDepsHasher := newHash() - HashRuleCommon(target.Rule, noDepsHasher) - hashWithoutDeps = noDepsHasher.Sum(nil) + hashWithoutDeps = target.HashWithoutDeps + if len(hashWithoutDeps) == 0 { + // Fallback for synthetic targets that bypass toTarget (shouldn't happen in practice). + noDepsHasher := newHash() + HashRuleCommon(target.Rule, noDepsHasher) + hashWithoutDeps = noDepsHasher.Sum(nil) + } h.Write(hashWithoutDeps) for _, dep := range target.Deps { depParam := p diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index bea74bf..3b86cda 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -191,6 +191,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget GitClient: gitModule, Config: repoCfg, ExtraExcludedFiles: param.Req.GetRequestOptions().GetExtraExcludeFilesRegex(), + Scope: b.scope, }) } result, err := runner.Compute(ctx, ws) From 6facd1832b3bfb0feea8c9db2598f60746ebaf4d Mon Sep 17 00:00:00 2001 From: yushan Date: Fri, 5 Jun 2026 23:33:10 +0000 Subject: [PATCH 2/2] Add m3 metrics --- graphrunner/BUILD.bazel | 1 + graphrunner/native.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/graphrunner/BUILD.bazel b/graphrunner/BUILD.bazel index 1b2953a..ed50781 100644 --- a/graphrunner/BUILD.bazel +++ b/graphrunner/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//core/git", "//core/targethasher", "//core/workspace", + "@com_github_uber_go_tally//:tally", ], ) diff --git a/graphrunner/native.go b/graphrunner/native.go index 0ee2615..cf5582d 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -16,7 +16,9 @@ package graphrunner import ( "context" + "time" + "github.com/uber-go/tally" "github.com/uber/tango/config" "github.com/uber/tango/core/bazel" "github.com/uber/tango/core/git" @@ -29,6 +31,7 @@ type nativeGraphRunner struct { git git.Interface config config.RepositoryConfig extraExcludedFiles []string + scope tally.Scope } type NativeGraphRunnerParams struct { @@ -36,15 +39,21 @@ type NativeGraphRunnerParams struct { GitClient git.Interface Config config.RepositoryConfig ExtraExcludedFiles []string + Scope tally.Scope } // graph runner takes in a bazel query request and computes the graph func NewNativeGraphRunner(p NativeGraphRunnerParams) GraphRunner { + scope := p.Scope + if scope == nil { + scope = tally.NoopScope + } return &nativeGraphRunner{ bazel: p.BazelClient, git: p.GitClient, config: p.Config, extraExcludedFiles: p.ExtraExcludedFiles, + scope: scope.SubScope("graph_runner"), } } @@ -57,6 +66,8 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) []string{"--order_output=no", "--proto:locations", "--noproto:default_values"}, g.config.BazelExtraArgs..., ) + + bazelStart := time.Now() queryResult, err := g.bazel.ExecuteQuery(ctx, &bazel.QueryRequest{ Query: query, // --order_output=no will make Bazel execute query faster @@ -67,13 +78,18 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) AdditionalArgs: additionalArgs, }) + g.scope.Timer("bazel_query_duration").Record(time.Since(bazelStart)) if err != nil { return targethasher.EmptyResult(), err } + + gitStart := time.Now() knownSourceHashes, err := g.git.FileHashes(ctx, "HEAD") + g.scope.Timer("git_file_hashes_duration").Record(time.Since(gitStart)) if err != nil { return targethasher.EmptyResult(), err } + hashConfig := targethasher.HashConfig{ KnownSourceHashes: knownSourceHashes, FullHashRepos: g.config.FullHashRepos, @@ -81,9 +97,13 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) UseBzlmod: g.config.BzlmodEnabled, } + hashStart := time.Now() res, err := targethasher.FromProto(ctx, queryResult.Result, ws.Path(), hashConfig) + g.scope.Timer("target_hash_duration").Record(time.Since(hashStart)) if err != nil { return targethasher.EmptyResult(), err } + + g.scope.Gauge("target_count").Update(float64(len(res.Targets))) return res, nil }