From e00b429089aff0b46f91fabdda7c67d96f986e3a Mon Sep 17 00:00:00 2001 From: Tetsuya KIKUCHI <97105818+t-kikuc@users.noreply.github.com> Date: Mon, 23 Dec 2024 16:46:43 +0900 Subject: [PATCH] Enhance logging in EventWatcher (#5443) * Add logger.Error() before returning for detailed logs Signed-off-by: t-kikuc * Enhance error logging in event watcher with additional context for push and commit failures Signed-off-by: t-kikuc --------- Signed-off-by: t-kikuc --- pkg/app/piped/eventwatcher/eventwatcher.go | 68 +++++++++++++++------- 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/pkg/app/piped/eventwatcher/eventwatcher.go b/pkg/app/piped/eventwatcher/eventwatcher.go index 97a9786a7a..90f24bee89 100644 --- a/pkg/app/piped/eventwatcher/eventwatcher.go +++ b/pkg/app/piped/eventwatcher/eventwatcher.go @@ -126,7 +126,8 @@ func (w *watcher) Run(ctx context.Context) error { workingDir, err := os.MkdirTemp("", "event-watcher") if err != nil { - return fmt.Errorf("failed to create the working directory: %w", err) + w.logger.Error("failed to create the working directory", zap.Error(err)) + return err } defer os.RemoveAll(workingDir) w.workingDir = workingDir @@ -134,6 +135,7 @@ func (w *watcher) Run(ctx context.Context) error { for _, r := range w.config.Repositories { repo, err := w.cloneRepo(ctx, r) if err != nil { + w.logger.Error("failed to clone repository", zap.String("repo-id", r.RepoID), zap.Error(err)) return err } defer repo.Clean() @@ -303,11 +305,13 @@ func (w *watcher) run(ctx context.Context, repo git.Repo, repoCfg config.PipedRe func (w *watcher) cloneRepo(ctx context.Context, repoCfg config.PipedRepository) (git.Repo, error) { dst, err := os.MkdirTemp(w.workingDir, repoCfg.RepoID) if err != nil { - return nil, fmt.Errorf("failed to create a new temporary directory: %w", err) + w.logger.Error("failed to create a new temporary directory", zap.Error(err)) + return nil, err } repo, err := w.gitClient.Clone(ctx, repoCfg.RepoID, repoCfg.Remote, repoCfg.Branch, dst) if err != nil { - return nil, fmt.Errorf("failed to clone repository %s: %w", repoCfg.RepoID, err) + w.logger.Error("failed to clone repository", zap.String("repo-id", repoCfg.RepoID), zap.Error(err)) + return nil, err } return repo, nil } @@ -317,11 +321,13 @@ func (w *watcher) execute(ctx context.Context, repo git.Repo, repoID string, eve // Copy the repo to another directory to modify local file to avoid reverting previous changes. tmpDir, err := os.MkdirTemp(w.workingDir, "repo") if err != nil { - return fmt.Errorf("failed to create a new temporary directory: %w", err) + w.logger.Error("failed to create a new temporary directory", zap.Error(err)) + return err } tmpRepo, err := repo.CopyToModify(filepath.Join(tmpDir, "tmp-repo")) if err != nil { - return fmt.Errorf("failed to copy the repository to the temporary directory: %w", err) + w.logger.Error("failed to copy the repository to the temporary directory", zap.Error(err)) + return err } // nolint: errcheck defer tmpRepo.Clean() @@ -368,7 +374,8 @@ func (w *watcher) execute(ctx context.Context, repo git.Repo, repoID string, eve Labels: matcher.Labels, }) if err != nil { - return fmt.Errorf("failed to get the latest event: %w", err) + w.logger.Error("failed to get the latest event", zap.Error(err)) + return err } // The case where the latest event has already been handled. if resp.Event.CreatedAt > latestEvent.CreatedAt { @@ -429,7 +436,8 @@ func (w *watcher) execute(ctx context.Context, repo git.Repo, repoID string, eve } if len(outDatedEvents) > 0 { if _, err := w.apiClient.ReportEventStatuses(ctx, &pipedservice.ReportEventStatusesRequest{Events: outDatedEvents}); err != nil { - return fmt.Errorf("failed to report event statuses: %w", err) + w.logger.Error("failed to report event statuses", zap.Error(err)) + return err } w.logger.Info(fmt.Sprintf("successfully made %d events OUTDATED", len(outDatedEvents))) } @@ -449,8 +457,11 @@ func (w *watcher) execute(ctx context.Context, repo git.Repo, repoID string, eve retry := backoff.NewRetry(retryPushNum, backoff.NewConstant(retryPushInterval)) for branch, events := range branchHandledEvents { _, err = retry.Do(ctx, func() (interface{}, error) { - err := tmpRepo.Push(ctx, branch) - return nil, err + if err := tmpRepo.Push(ctx, branch); err != nil { + w.logger.Error("failed to push commits", zap.String("repo-id", repoID), zap.String("branch", branch), zap.Error(err)) + return nil, err + } + return nil, nil }) if err == nil { @@ -493,11 +504,13 @@ func (w *watcher) updateValues(ctx context.Context, repo git.Repo, repoID string // Copy the repo to another directory to modify local file to avoid reverting previous changes. tmpDir, err := os.MkdirTemp(w.workingDir, "repo") if err != nil { - return fmt.Errorf("failed to create a new temporary directory: %w", err) + w.logger.Error("failed to create a new temporary directory", zap.Error(err)) + return err } tmpRepo, err := repo.CopyToModify(filepath.Join(tmpDir, "tmp-repo")) if err != nil { - return fmt.Errorf("failed to copy the repository to the temporary directory: %w", err) + w.logger.Error("failed to copy the repository to the temporary directory", zap.Error(err)) + return err } defer tmpRepo.Clean() @@ -536,7 +549,8 @@ func (w *watcher) updateValues(ctx context.Context, repo git.Repo, repoID string Labels: e.Labels, }) if err != nil { - return fmt.Errorf("failed to get the latest event: %w", err) + w.logger.Error("failed to get the latest event", zap.Error(err)) + return err } // The case where the latest event has already been handled. if resp.Event.CreatedAt > latestEvent.CreatedAt { @@ -577,7 +591,8 @@ func (w *watcher) updateValues(ctx context.Context, repo git.Repo, repoID string } if len(outDatedEvents) > 0 { if _, err := w.apiClient.ReportEventStatuses(ctx, &pipedservice.ReportEventStatusesRequest{Events: outDatedEvents}); err != nil { - return fmt.Errorf("failed to report event statuses: %w", err) + w.logger.Error("failed to report event statuses", zap.Error(err)) + return err } w.logger.Info(fmt.Sprintf("successfully made %d events OUTDATED", len(outDatedEvents))) } @@ -587,12 +602,16 @@ func (w *watcher) updateValues(ctx context.Context, repo git.Repo, repoID string retry := backoff.NewRetry(retryPushNum, backoff.NewConstant(retryPushInterval)) _, err = retry.Do(ctx, func() (interface{}, error) { - err := tmpRepo.Push(ctx, tmpRepo.GetClonedBranch()) - return nil, err + if err := tmpRepo.Push(ctx, tmpRepo.GetClonedBranch()); err != nil { + w.logger.Error("failed to push commits", zap.String("repo-id", repoID), zap.String("branch", tmpRepo.GetClonedBranch()), zap.Error(err)) + return nil, err + } + return nil, nil }) if err == nil { if _, err := w.apiClient.ReportEventStatuses(ctx, &pipedservice.ReportEventStatusesRequest{Events: handledEvents}); err != nil { - return fmt.Errorf("failed to report event statuses: %w", err) + w.logger.Error("failed to report event statuses", zap.Error(err)) + return err } w.milestoneMap.Store(repoID, maxTimestamp) return nil @@ -613,10 +632,12 @@ func (w *watcher) updateValues(ctx context.Context, repo git.Repo, repoID string handledEvents[i].StatusDescription = fmt.Sprintf("Failed to push changed files: %v", err) } if _, err := w.apiClient.ReportEventStatuses(ctx, &pipedservice.ReportEventStatusesRequest{Events: handledEvents}); err != nil { - return fmt.Errorf("failed to report event statuses: %w", err) + w.logger.Error("failed to report event statuses: %w", zap.Error(err)) + return err } w.milestoneMap.Store(repoID, maxTimestamp) - return fmt.Errorf("failed to push commits: %w", err) + w.logger.Error("failed to push commits", zap.Error(err)) + return err } // commitFiles commits changes if the data in Git is different from the latest event. @@ -647,6 +668,7 @@ func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eve newContent, upToDate, err = modifyText(path, r.Regex, latestEvent.Data) } if err != nil { + w.logger.Error("failed to modify file", zap.Error(err)) return "", err } if upToDate { @@ -654,7 +676,8 @@ func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eve } if err := os.WriteFile(path, newContent, os.ModePerm); err != nil { - return "", fmt.Errorf("failed to write file: %w", err) + w.logger.Error("failed to write file", zap.Error(err)) + return "", err } changes[filePath] = newContent } @@ -670,7 +693,12 @@ func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eve branch := makeBranchName(newBranch, eventName, repo.GetClonedBranch()) trailers := maps.Clone(latestEvent.Contexts) if err := repo.CommitChanges(ctx, branch, commitMsg, newBranch, changes, trailers); err != nil { - return "", fmt.Errorf("failed to perform git commit: %w", err) + w.logger.Error("failed to perform git commit", + zap.String("branch", branch), + zap.Bool("make-new-branch", newBranch), + zap.Int("changed-files", len(changes)), + zap.Error(err)) + return "", err } w.logger.Info(fmt.Sprintf("event watcher will update values of Event %q", eventName)) return branch, nil