diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index 02bd061..1014984 100644 --- a/controller/getchangedtargets.go +++ b/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 != "" { diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index 3ead896..fa0450e 100644 --- a/controller/gettargetgraph.go +++ b/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)) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index 5a5cdd3..4b13886 100644 --- a/core/bazel/bazel.go +++ b/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, diff --git a/core/bazel/query.go b/core/bazel/query.go index d8f6e7c..37f10a7 100644 --- a/core/bazel/query.go +++ b/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) } diff --git a/core/workspace/gitrequest.go b/core/workspace/gitrequest.go index eb23733..7432848 100644 --- a/core/workspace/gitrequest.go +++ b/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 diff --git a/orchestrator/errors.go b/orchestrator/errors.go index dd82381..9b8c039 100644 --- a/orchestrator/errors.go +++ b/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" -) diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index 2737e79..58d7ac5 100644 --- a/orchestrator/native_orchestrator.go +++ b/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