Skip to content

fix: add some logs to flaky handlers#4978

Merged
jtmcdole merged 1 commit into
mainfrom
logsForFlaky
Mar 10, 2026
Merged

fix: add some logs to flaky handlers#4978
jtmcdole merged 1 commit into
mainfrom
logsForFlaky

Conversation

@jtmcdole

Copy link
Copy Markdown
Member

While testing today, I realized I can't see anything from the cloud logs.

While testing today, I realized I can't see anything from the cloud
logs.
@jtmcdole jtmcdole requested a review from matanlurey March 10, 2026 15:33
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Hi @jtmcdole, I've received your request, and I'm working on it now! You can track my progress in the logs for more details.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

## 📋 Review Summary

This PR introduces helpful logging to the flaky test handling mechanisms to improve observability. The additions are targeted and achieve the stated objective.

🔍 General Feedback

  • The log additions are clear and context-rich.
  • Consider avoiding logging full objects like the Issue class within collections to prevent excessive log bloat, as suggested in the inline comment.

Comment on lines +163 to +164
log.info(
'updateExistingFlakyIssue($slug): handling $nameToExistingIssue over ${builderFlakyMap.length} builders (${ignoreFlakyMap.length} ignored)',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Logging the entire `nameToExistingIssue` map, which contains `Issue` objects as values, may result in verbose and hard-to-read logs since it will print the `toString()` representation of each `Issue`. Consider logging just the keys (builder names) or the total count to keep the logs clean and informative.
Suggested change
log.info(
'updateExistingFlakyIssue($slug): handling $nameToExistingIssue over ${builderFlakyMap.length} builders (${ignoreFlakyMap.length} ignored)',
log.info(
'updateExistingFlakyIssue($slug): handling ${nameToExistingIssue.keys} over ${builderFlakyMap.length} builders (${ignoreFlakyMap.length} ignored)',
);

@jtmcdole jtmcdole merged commit ad73c50 into main Mar 10, 2026
31 checks passed
@jtmcdole jtmcdole deleted the logsForFlaky branch April 16, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants