-
Notifications
You must be signed in to change notification settings - Fork 6.5k
chore(log): change high frequency logs to DEBUG level #25092
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.
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
Conversation
Signed-off-by: Peter Jiang <[email protected]>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #25092 +/- ##
==========================================
+ Coverage 60.89% 62.22% +1.33%
==========================================
Files 351 351
Lines 60489 49213 -11276
==========================================
- Hits 36832 30624 -6208
+ Misses 20736 15660 -5076
- Partials 2921 2929 +8 ☔ View full report in Codecov by Sentry. |
| "time_ms": reconcileDuration.Milliseconds(), | ||
| "patch_ms": patchDuration.Milliseconds(), | ||
| "setop_ms": setOpDuration.Milliseconds(), | ||
| }).Info("Reconciliation completed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i personally look at the times that are emitted by this log message. they can be helpful in seeing where the application controller is spending its time during a reconcile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rumstead Since i'm changing this log line to DEBUG it will still be available, but users need to turn on debug logs.
Would that be sufficient or do you feel like this is a must have log as INFO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the logging to debug is a reactive step. I see the log line going back to 2019. Why do we feel it's too verbose now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it is more verbose because of this change https://github.com/argoproj/argo-cd/pull/21442/files#diff-9312ec23ecb354e9d5c3c7434a6b0d37f6c5d780153e7f1f28221af48ddde453
that added extra fields to this logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that just adding 3 additional fields? project, namespace, and application name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, those additional field will make the log line longer, but you're right its likely not significant enough to cause the volume increase.
I think we can go ahead and close this ticket as these logs are worth keeping. Thanks for the review anyway @rumstead
Signed-off-by: Peter Jiang <[email protected]>
fixes #25093
We noticed 2x increase in log volume likely due to this change: https://github.com/argoproj/argo-cd/pull/21442/files#diff-9312ec23ecb354e9d5c3c7434a6b0d37f6c5d780153e7f1f28221af48ddde453
I think
logCtx.Info("Reconciliation completed")should be DEBUG level becauseargocd_app_reconcilemetrics to track this stat alreadySame with
logCtx.Info("GetRepoObjs stats")- i don't see too much value in having this as log level INFO as well. This is also contributing to high volume as it appears on every reconcile.Checklist: