Clean up logging and remove double-logged errors

controller/getchangedtargets.go

@@ -52,7 +52,6 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str
 		}
 	}()
 	if err := validateGetChangedTargetsRequest(request); err != nil {
-		c.logger.Error("GetChangedTargets: Invalid request", zap.Error(err))
 		return common.WithReason(common.FailureReasonValidation, common.ErrorTypeUser, err)
 	}
 	scope = scope.Tagged(map[string]string{"repo": common.ToShortRemote(request.GetFirstRevision().GetRemote())})
@@ -290,7 +289,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str
 			// Goroutine outlives the handler so we can't return; log loudly and
 			// abandon the cache write. Surfacing infra failures matters more than
 			// a missed cache opportunity.
-			logger.Error("GetChangedTargets: skipping cache write, failed to read revision treehash", zap.Error(err))
+			logger.Warn("GetChangedTargets: skipping cache write, failed to read revision treehash", zap.Error(err))
 			return
 		}
 		if treehash1 != "" && treehash2 != "" {

controller/gettargetgraph.go

@@ -104,7 +104,7 @@ func (c *controller) getGraph(ctx context.Context, buildDescription *pb.BuildDes
 		if err != nil {
 			if storage.IsNotFound(err) {
 				// Cache miss - blob doesn't exist, need to compute and store target graph
-				logger.Info("getGraph: treehash not found", zap.Error(err))
+				logger.Debug("getGraph: treehash not found", zap.Error(err))
 			} else {
 				// Other errors (network, infra issues) should be retried
 				logger.Error("getGraph: Storage error", zap.Error(err))

core/bazel/bazel.go

@@ -88,10 +88,9 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) {
 	}
 	bazelCommand, err := detectBazelExecutable(ctx, p.BazelCommand)
 	if err != nil {
-		p.Logger.Errorw("NewBazelClient: Error detecting bazel executable", zap.Error(err))
-		return nil, err
+		return nil, fmt.Errorf("detect bazel executable: %w", err)
 	}
-	p.Logger.Info("NewBazelClient", zap.String("bazelCommand", bazelCommand), zap.String("workspacePath", p.WorkspacePath))
+	p.Logger.Debugw("NewBazelClient", zap.String("bazelCommand", bazelCommand), zap.String("workspacePath", p.WorkspacePath))
 	return &BazelClient{
 		workspacePath:      p.WorkspacePath,
 		envVarsMap:         p.EnvVarsMap,

core/bazel/query.go

@@ -37,7 +37,7 @@ func (b *BazelClient) setupCommand(ctx context.Context, query string, startupOpt
 	args = append(args, additionalArgs...)
 	args = append(args, "--output=streamed_proto")
 	args = append(args, query)
-	b.logger.Infow("Querying Bazel", zap.String("workspacePath", b.workspacePath), zap.String("query", query))
+	b.logger.Debugw("Querying Bazel", zap.String("workspacePath", b.workspacePath), zap.String("query", query))
 	return b.execCommandContext(ctx, b.bazelCommand, args...)
 }
 
@@ -100,7 +100,7 @@ func (b *BazelClient) executeQueryInternal(ctx context.Context, query string, st
 	if streamErr != nil {
 		return nil, b.wrapQueryFailure("stream processing failed", streamErr, &stderrBuf)
 	}
-	b.logger.Debugf("Parsed %d targets from bazel query", len(queryResults.Target))
+	b.logger.Debugw("Parsed targets from bazel query", zap.Int("target_count", len(queryResults.Target)))
 	return queryResults, nil
 }
 
@@ -113,7 +113,6 @@ func (b *BazelClient) wrapQueryFailure(msg string, cause error, stderrBuf *bytes
 	if !b.streamLogs {
 		tail = "\nstderr:\n" + stderrBuf.String()
 	}
-	b.logger.Errorf("%s: %v%s", msg, cause, tail)
 	return fmt.Errorf("%s: %w%s", msg, cause, tail)
 }
 

core/workspace/gitrequest.go

@@ -49,8 +49,7 @@ func (r *gitRequest) Apply(ctx context.Context) error {
 	ref := fmt.Sprintf("+pull/%s/head:pull/%s/head", r.requestID, r.requestID)
 	err := r.git.Fetch(ctx, "origin", ref, "--force", "--no-tags")
 	if err != nil {
-		r.logger.Errorw("gitRequest: Failed to fetch PR", zap.String("request_id", r.requestID), zap.Error(err))
-		return err
+		return fmt.Errorf("fetch PR %s: %w", r.requestID, err)
 	}
 	if r.commit != "" {
 		isAncestor, err := r.git.IsAncestor(ctx, r.commit, fmt.Sprintf("pull/%s/head", r.requestID))
@@ -63,23 +62,19 @@ func (r *gitRequest) Apply(ctx context.Context) error {
 	}
 	patch, err := r.git.Diff(ctx, r.baseRef, fmt.Sprintf("pull/%s/head", r.requestID), "--binary", "--merge-base")
 	if err != nil {
-		r.logger.Errorw("gitRequest: Failed to compute diff", zap.String("request_id", r.requestID), zap.Error(err))
-		return err
+		return fmt.Errorf("compute diff for PR %s: %w", r.requestID, err)
 	}
 	err = r.git.ApplyPatch(ctx, patch)
 	if err != nil {
-		r.logger.Errorw("gitRequest: Failed to apply patch", zap.String("request_id", r.requestID), zap.Error(err))
-		return err
+		return fmt.Errorf("apply patch for PR %s: %w", r.requestID, err)
 	}
 	err = r.git.Commit(ctx, fmt.Sprintf("Applied PR: %s", r.requestID), "--allow-empty")
 	if err != nil {
-		r.logger.Errorw("gitRequest: Failed to commit", zap.String("request_id", r.requestID), zap.Error(err))
-		return err
+		return fmt.Errorf("commit PR %s: %w", r.requestID, err)
 	}
 	err = r.git.SubmoduleUpdate(ctx)
 	if err != nil {
-		r.logger.Errorw("gitRequest: Failed to update submodules", zap.String("request_id", r.requestID), zap.Error(err))
-		return err
+		return fmt.Errorf("update submodules for PR %s: %w", r.requestID, err)
 	}
 	r.logger.Infow("gitRequest: Successfully applied PR", zap.String("request_id", r.requestID))
 	return nil

orchestrator/errors.go

@@ -13,18 +13,3 @@
 // limitations under the License.
 
 package orchestrator
-
-// failure_reason tag values emitted by the orchestrator.
-// Shared reasons live in core/common as common.FailureReason*.
-const (
-	failureReasonConfigParse       = "config_parse"
-	failureReasonNoRepoConfig      = "no_repo_config"
-	failureReasonWorkspaceLease    = "workspace_lease"
-	failureReasonWorkspaceCheckout = "workspace_checkout"
-	failureReasonRequestCreate     = "request_create"
-	failureReasonRequestApply      = "request_apply"
-	failureReasonTreehashCompute   = "treehash_compute"
-	failureReasonBazelClient       = "bazel_client"
-	failureReasonGraphCompute      = "graph_compute"
-	failureReasonGraphConvert      = "graph_convert"
-)

orchestrator/native_orchestrator.go

@@ -116,32 +116,29 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget
 	// parse the config file
 	cfg, err := config.Parse(b.configFilePath)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error parsing config file", zap.String("configFilePath", b.configFilePath), zap.Error(err))
-		return nil, common.WithReason(failureReasonConfigParse, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("parse config %q: %w", b.configFilePath, err)
 	}
 	remote := param.Req.BuildDescription.Remote
 	repoCfg, ok := cfg.GetRepositoryConfig(remote)
 	if !ok {
-		return nil, common.WithReason(failureReasonNoRepoConfig, common.ErrorTypeUser, fmt.Errorf("no repository configuration found for remote %q", remote))
+		return nil, fmt.Errorf("no repository configuration found for remote %q", remote)
 	}
 	ws, err := b.repoManager.Lease(ctx, *param.Req.BuildDescription)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error leasing workspace", zap.Error(err))
-		return nil, common.WithReason(failureReasonWorkspaceLease, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("lease workspace: %w", err)
 	}
 	defer func() {
 		err := ws.Release()
 		if err != nil {
 			// clean up the workspace if release fails.
 			if removeErr := os.RemoveAll(ws.Path()); removeErr != nil {
-				logger.Errorf("GetTargetGraph: Failed to remove workspace: %v", removeErr)
+				logger.Errorw("GetTargetGraph: Failed to remove workspace", zap.Error(removeErr))
 			}
 		}
 	}()
 	err = ws.Checkout(ctx, param.Req.BuildDescription.Remote, param.Req.BuildDescription.BaseSha)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error checking out base revision", zap.Error(err))
-		return nil, common.WithReason(failureReasonWorkspaceCheckout, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("checkout %s@%s: %w", param.Req.BuildDescription.Remote, param.Req.BuildDescription.BaseSha, err)
 	}
 	logger.Infow("GetTargetGraph: Checked out base revision")
 
@@ -155,23 +152,20 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget
 	for _, req := range param.Req.BuildDescription.Requests {
 		request, err := workspace.NewRequest(req.GetUrl(), gitModule, param.Req.BuildDescription.BaseSha, req.GetCommit(), logger)
 		if err != nil {
-			logger.Errorw("GetTargetGraph: Error creating request", zap.String("url", req.GetUrl()), zap.Error(err))
-			return nil, common.WithReason(failureReasonRequestCreate, common.ErrorTypeInfra, err)
+			return nil, fmt.Errorf("create request for %q: %w", req.GetUrl(), err)
 		}
 		requests = append(requests, request)
 	}
 	err = ws.ApplyRequests(ctx, requests)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error applying requests to workspace", zap.Error(err))
-		return nil, common.WithReason(failureReasonRequestApply, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("apply requests: %w", err)
 	}
 	logger.Infow("GetTargetGraph: Applied requests", zap.Int("request_count", len(requests)))
 
 	// Compute the treehash and download the target graph from storage if exists.
 	treehash, err := gitModule.RevParse(ctx, "HEAD^{tree}")
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Treehash computation failed", zap.Error(err))
-		return nil, common.WithReason(failureReasonTreehashCompute, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("compute treehash: %w", err)
 	}
 	treehashPath := common.GetGraphByTreeHash(param.Req.BuildDescription.Remote, treehash, param.Req.BuildDescription.GetStrategy(), param.Req.GetRequestOptions())
 	if !param.BypassCache {
@@ -181,8 +175,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget
 			return graphReader, nil
 		}
 		if !storage.IsNotFound(err) {
-			logger.Errorw("GetTargetGraph: Storage error", zap.Error(err))
-			return nil, common.WithReason(common.FailureReasonStorage, common.ErrorTypeInfra, err)
+			return nil, fmt.Errorf("read graph at treehash %s: %w", treehash, err)
 		}
 		logger.Infow("GetTargetGraph: Treehash not found, computing target graph", zap.String("treehash", treehash))
 	} else {
@@ -199,8 +192,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget
 			StreamLogs:    repoCfg.StreamBazelLogs,
 		})
 		if err != nil {
-			logger.Errorw("GetTargetGraph: Error creating bazel client", zap.Error(err))
-			return nil, common.WithReason(failureReasonBazelClient, common.ErrorTypeInfra, err)
+			return nil, fmt.Errorf("create bazel client: %w", err)
 		}
 		// Use default native graph runner
 		runner = graphrunner.NewNativeGraphRunner(graphrunner.NativeGraphRunnerParams{
@@ -213,30 +205,25 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget
 	}
 	result, err := runner.Compute(ctx, ws)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error computing target graph", zap.Error(err))
-		return nil, common.WithReason(failureReasonGraphCompute, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("compute target graph: %w", err)
 	}
 	responses, err := common.ResultToGetTargetGraphResponse(ctx, result)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error converting target graph to response", zap.Error(err))
-		return nil, common.WithReason(failureReasonGraphConvert, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("convert target graph to response: %w", err)
 	}
 	err = storage.WriteGraphStream(ctx, b.storage, treehashPath, responses)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error writing target graph to storage", zap.Error(err))
-		return nil, common.WithReason(common.FailureReasonStorage, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("write graph to storage at %s: %w", treehashPath, err)
 	}
 	treehashCachePath := common.GetTreehashCachePath(param.Req.BuildDescription)
 	treehashReader := bytes.NewReader([]byte(treehash))
 	err = b.storage.Put(ctx, storage.UploadRequest{Key: treehashCachePath, Reader: treehashReader})
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error storing treehash mapping", zap.Error(err))
-		return nil, common.WithReason(common.FailureReasonStorage, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("store treehash mapping at %s: %w", treehashCachePath, err)
 	}
 	graphReader, err := storage.NewGraphReader(ctx, b.storage, treehashPath)
 	if err != nil {
-		logger.Errorw("GetTargetGraph: Error creating graph reader", zap.Error(err))
-		return nil, common.WithReason(common.FailureReasonStorage, common.ErrorTypeInfra, err)
+		return nil, fmt.Errorf("create graph reader at %s: %w", treehashPath, err)
 	}
 	logger.Infow("GetTargetGraph: Done computing and storing target graph", zap.String("treehash", treehash))
 	return graphReader, nil