Skip to content

chore: add logs with timing for cluster sync performance#28707

Open
agaudreault wants to merge 1 commit into
argoproj:masterfrom
agaudreault:log-cache-sync
Open

chore: add logs with timing for cluster sync performance#28707
agaudreault wants to merge 1 commit into
argoproj:masterfrom
agaudreault:log-cache-sync

Conversation

@agaudreault

@agaudreault agaudreault commented Jul 13, 2026

Copy link
Copy Markdown
Member

To be able to have more insights on the cache sync performance, add timing stats to the log message.

This will help diagnose performance issues with the cache holding a lock and blocking all reconciliations while a cluster is syncing.

Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
@agaudreault agaudreault requested a review from a team as a code owner July 13, 2026 21:09
@bunnyshell

bunnyshell Bot commented Jul 13, 2026

Copy link
Copy Markdown

❗ Preview Environment deployment failed on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

@agaudreault agaudreault enabled auto-merge (squash) July 13, 2026 21:10
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

return nil
})
})
log := c.log.WithValues(

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.

For cases where there is a long sync that eventually fails, we may lose the timing log.
What do you think about wrapping this in a defer func() so that we don't lose the log on early returns?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The code doesn't look really great with the defer since we don't have a nice object like TimingStats in the gitops-engine submodule, and that would change where the "Failed to sync cluster" error is logged.

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