Skip to content

feat: add metric for sync durations(#11675) #18474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 16, 2025
Merged

feat: add metric for sync durations(#11675) #18474

merged 1 commit into from
Jun 16, 2025

Conversation

Jack-R-lantern
Copy link
Contributor

@Jack-R-lantern Jack-R-lantern commented Jun 2, 2024 •
edited
Loading

Added a metric to measure application sync duration.
It is implemented using summary, and the average can be calculated using count, sum.

Closes #11675

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@codecov Codecov
Copy link

codecov bot commented Jun 2, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.09%. Comparing base (28e871e) to head (1f21776).
Report is 15 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #18474      +/-   ##
==========================================
- Coverage   60.11%   60.09%   -0.02%     
==========================================
  Files         342      342              
  Lines       58820    58829       +9     
==========================================
- Hits        35359    35353       -6     
- Misses      20617    20623       +6     
- Partials     2844     2853       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch 3 times, most recently from e2eef6a to 00b91ec Compare June 4, 2024 13:38
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch 2 times, most recently from a3e7df0 to 19ce59e Compare July 10, 2024 01:20
@Munken
Copy link

Munken commented Aug 27, 2024

Just got hit by long sync times and would like to setup monitoring for this. What is missing in this PR? Would you like a hand @Jack-R-lantern ?

sethgupton-mastery added a commit to sethgupton-mastery/argo-cd that referenced this pull request Sep 19, 2024
@sethgupton-mastery
Sync Duration metrics
cd798f1 
From argoproj#18474
andrii-korotkov-verkada
andrii-korotkov-verkada reviewed Nov 9, 2024
View reviewed changes
Copy link
Contributor

@andrii-korotkov-verkada andrii-korotkov-verkada left a comment

Choose a reason for hiding this comment

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

Can you add some tests?

Also, note that there are logs available with timing information about specific syncs. There's also longest running processor duration metric for syncs as well.

controller/metrics/metrics.go Outdated
// Help: "Application sync performance in seconds.",
// },
// append(descAppDefaultLabels, "dest_server"),
// )
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this

@sethgupton-mastery
Copy link
Contributor

We ended up grabbing these changes and making a build with these changes to run on our Argo instances. It has been working great.
Would love for this to get merged into the main line.

@bogedal
Copy link

bogedal commented Jan 29, 2025

I agree with @sethgupton-mastery - would also like to see this added @andrii-korotkov-verkada

@andrii-korotkov-verkada
Copy link
Contributor

Let's rebase/merge latest master and productionize it more, e.g. remove commented code and make it not a draft.

@bogedal
Copy link

bogedal commented Jan 29, 2025

Let's rebase/merge latest master and productionize it more, e.g. remove commented code and make it not a draft.

Something you will look into @Jack-R-lantern ?

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from 19ce59e to 448c1f7 Compare January 29, 2025 08:14
@Jack-R-lantern Jack-R-lantern marked this pull request as ready for review January 29, 2025 08:15
@Jack-R-lantern Jack-R-lantern requested a review from a team as a code owner January 29, 2025 08:15
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from 448c1f7 to 2d10342 Compare January 29, 2025 09:29
@Jack-R-lantern
Copy link
Contributor Author

@andrii-korotkov-verkada @bogedal @sethgupton-mastery

I implemented syncDurationTotal as a Counter.
For summary, histogram, there is a risk of cardinality explosion.
So I think it makes sense to track SyncDuration through metric and rate operations of Counter type, but what do you think?

@Munken
Copy link

Munken commented Jan 29, 2025

I implemented syncDurationTotal as a Counter.

This should be fine since syncCounter is available.

Munken
Munken approved these changes Jan 29, 2025
View reviewed changes
Copy link

@Munken Munken left a comment

Choose a reason for hiding this comment

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

LGTM

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from 2d10342 to be773ae Compare February 6, 2025 14:00
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from be773ae to f682b26 Compare March 10, 2025 14:51
@miguelvr
Copy link

I would really like this change to land. What's missing to get this in?

@uristernik
Copy link

Gentle ping here 🙏

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from f682b26 to 4371bf6 Compare April 9, 2025 13:34
@Jack-R-lantern
Copy link
Contributor Author

@andrii-korotkov-verkada please review

@uristernik
Copy link

@andrii-korotkov-verkada @Jack-R-lantern
Can I help expedite this in somehow?

@andrii-korotkov-verkada
Copy link
Contributor

Bring this up at the contributors meeting on Thursdays.

alexmt
alexmt approved these changes Jun 12, 2025
View reviewed changes
Copy link
Collaborator

@alexmt alexmt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes looks good to me. The only concern is potentially too much metrics (because of app name tag) but it was solved by metrics reset support.

So LGTM. Will double check in contributors meeting tomorrow and merge if no objections

agaudreault
agaudreault reviewed Jun 13, 2025
View reviewed changes
agaudreault
agaudreault requested changes Jun 13, 2025
View reviewed changes
@agaudreault agaudreault added this to the v3.1 milestone Jun 13, 2025
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from 4371bf6 to 56ec02f Compare June 14, 2025 08:52
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from 56ec02f to c1f47b5 Compare June 14, 2025 08:59
agaudreault
agaudreault requested changes Jun 16, 2025
View reviewed changes
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from c1f47b5 to ea3af5b Compare June 16, 2025 13:49
@Jack-R-lantern Jack-R-lantern requested a review from a team as a code owner June 16, 2025 13:49
@Jack-R-lantern
feat: add metric for sync durations
1f21776 
add syncDuration metric
add syncDuration metric unit test

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-11675/add_metric_for_sync_durations branch from ea3af5b to 1f21776 Compare June 16, 2025 15:29
@agaudreault agaudreault merged commit 98ca411 into argoproj:master Jun 16, 2025
28 checks passed
enneitex pushed a commit to enneitex/argo-cd that referenced this pull request Aug 24, 2025
@Jack-R-lantern @enneitex
feat: add metric for sync durations(argoproj#11675) (argoproj#18474)
ddc9e7c 
Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Signed-off-by: enneitex <etienne.divet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agaudreault agaudreault agaudreault approved these changes

@andrii-korotkov-verkada andrii-korotkov-verkada Awaiting requested review from andrii-korotkov-verkada

@alexmt alexmt Awaiting requested review from alexmt

+2 more reviewers

@Munken Munken Munken approved these changes

@uristernik uristernik uristernik approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1
Development

Successfully merging this pull request may close these issues.

Add metric for sync durations
9 participants
@Jack-R-lantern @Munken @sethgupton-mastery @bogedal @andrii-korotkov-verkada @miguelvr @uristernik @alexmt @agaudreault