chore(logs): standardize application logging (#20731) #21442

dhawos · 2025-01-09T22:20:52Z

I've tried implementing what was requested in #20731.

I moved the getAppLog function (that will return log.Fields when given an Application). From the application controller to the common utils. I'm not too sure if that's the right place for such a function so let me know if I should put it elsewhere.
I'm not sure I've found all the places where it could be used, but I searched through all usage of the log.WithField(s) methods and changed those that were logging application details.

I only changed places where an Application struct was available, I saw that in some places we had different structs (such as ApplicationQuery) but I wasn't sure that it was in scope so I did not change it.

I have not written any test since I'm not introducing any new logic but if you think some tests are relevant, I'd be glad to implement them.

Fixes #20731

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 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).
I have added a brief description of why this PR is necessary and/or what this PR solves.

bunnyshell · 2025-01-09T22:21:00Z

❗ Preview Environment undeploy from Bunnyshell failed

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to try again to remove the environment

crenshaw-dev · 2025-01-16T21:06:11Z

applicationset/controllers/applicationset_controller.go

 		GenericFunc: func(e event.GenericEvent) bool {
 			if log.IsLevelEnabled(log.DebugLevel) {
-				var appName string
+				logFields := log.Fields{"app": ""}


Suggested change

logFields := log.Fields{"app": ""}

logFields := log.Fields{}

Probably fine to start with an empty set and replace it if the type assertion is successful.

crenshaw-dev

Would you mind adding a quick note somewhere in the docs that these are standard log fields? Maybe in/around the "audit" documentation?

dhawos · 2025-01-21T22:35:37Z

Would you mind adding a quick note somewhere in the docs that these are standard log fields? Maybe in/around the "audit" documentation?

No problem. I'm not sure where you would like me to put this though. Are you referring to this page https://argo-cd.readthedocs.io/en/stable/operator-manual/security/#auditing ?

codecov · 2025-01-21T22:55:09Z

Codecov Report

❌ Patch coverage is 83.05085% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.95%. Comparing base (ec9b43f) to head (0352914).
⚠️ Report is 472 commits behind head on master.

Files with missing lines	Patch %	Lines
server/application/application.go	66.66%	4 Missing ⚠️
...cationset/controllers/applicationset_controller.go	75.00%	3 Missing ⚠️
controller/health.go	0.00%	1 Missing ⚠️
controller/hydrator/hydrator.go	50.00%	1 Missing ⚠️
server/application/broadcaster.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #21442      +/-   ##
==========================================
- Coverage   59.98%   59.95%   -0.03%     
==========================================
  Files         343      343              
  Lines       57846    57837       -9     
==========================================
- Hits        34699    34679      -20     
- Misses      20364    20372       +8     
- Partials     2783     2786       +3

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

crenshaw-dev · 2025-01-22T18:14:28Z

@dhawos yes I think that would be an okay place!

dhawos · 2025-01-22T19:37:10Z

@dhawos yes I think that would be an okay place!

Alright, I've added a quick note on those fields in the docs. Let me know if you would like any change.

dhawos · 2025-03-09T14:10:16Z

Just rebased as some conflicts emerged and ensured CI was green.

andrii-korotkov-verkada · 2025-03-14T13:47:20Z

@crenshaw-dev, can you TAL, please?

andrii-korotkov-verkada · 2025-04-18T15:23:41Z

Probably worth presenting at the contributors meeting on Thursdays to facilitate the review.

docs/operator-manual/security.md

andrii-korotkov-verkada · 2025-05-02T14:39:55Z

@dhawos, we got Michael ready to approve once the feedback is addressed. Can you follow-up when you have time, please?

andrii-korotkov-verkada · 2025-05-13T19:42:08Z

@dhawos, can you give some status update, please? It's okay if you don't have time right now to follow-up.

dhawos · 2025-05-16T12:01:09Z

@dhawos, can you give some status update, please? It's okay if you don't have time right now to follow-up.

Sorry I did not have access to my computer for some time. I'll try to resolve conflicts that emerged and address the feedback as soon as I can.

andrii-korotkov · 2025-05-16T16:42:16Z

util/app/log/log.go

+	appv1 "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
+)
+
+func GetAppLog(app *appv1.Application) log.Fields {


Let's name this GetAppLogFields to make it more clear.

Good point, I've adressed it in my latest commit

andrii-korotkov · 2025-05-16T16:42:30Z

util/app/log/log.go

+	return log.Fields{
+		"application":        app.Name,
+		"app-namespace":      app.Namespace,
+		"app-qualified-name": app.QualifiedName(),


Let's remove this per Michael's comment

Just removed it in my latest commit

Signed-off-by: Louis Gérard <[email protected]>

…oj#21442) Signed-off-by: Louis Gérard <[email protected]> Signed-off-by: Lyheng <[email protected]>

…oj#21442) Signed-off-by: Louis Gérard <[email protected]>

…oj#21442) Signed-off-by: Louis Gérard <[email protected]> Signed-off-by: dsuhinin <[email protected]>

dhawos marked this pull request as ready for review January 10, 2025 07:52

dhawos requested a review from a team as a code owner January 10, 2025 07:52

crenshaw-dev reviewed Jan 16, 2025

View reviewed changes

dhawos force-pushed the chore/standardize-app-logging branch 2 times, most recently from 9e268eb to ad9ebdb Compare January 21, 2025 21:20

crenshaw-dev reviewed Jan 21, 2025

View reviewed changes

dhawos force-pushed the chore/standardize-app-logging branch from ad9ebdb to b2fff65 Compare January 21, 2025 22:17

dhawos force-pushed the chore/standardize-app-logging branch from b2fff65 to 79f44c9 Compare January 22, 2025 19:35

dhawos requested a review from a team as a code owner January 22, 2025 19:35

andrii-korotkov-verkada approved these changes Mar 5, 2025

View reviewed changes

andrii-korotkov-verkada added the ready-for-review An approver should give a final review and merge the PR label Mar 5, 2025

github-project-automation bot added this to Argo CD Review Mar 5, 2025

github-project-automation bot moved this to Ready for final review in Argo CD Review Mar 5, 2025

andrii-korotkov-verkada requested review from crenshaw-dev, ishitasequeira and leoluz March 5, 2025 22:22

dhawos force-pushed the chore/standardize-app-logging branch 3 times, most recently from 33688d0 to ecbf780 Compare March 9, 2025 12:45

crenshaw-dev reviewed Apr 24, 2025

View reviewed changes

docs/operator-manual/security.md Outdated Show resolved Hide resolved

dhawos force-pushed the chore/standardize-app-logging branch from ecbf780 to 2120b4a Compare May 16, 2025 11:59

dhawos force-pushed the chore/standardize-app-logging branch 2 times, most recently from 7974056 to 498a38e Compare May 16, 2025 14:18

andrii-korotkov reviewed May 16, 2025

View reviewed changes

dhawos force-pushed the chore/standardize-app-logging branch 2 times, most recently from cda4d93 to ed83f45 Compare May 18, 2025 18:28

andrii-korotkov approved these changes May 18, 2025

View reviewed changes

dhawos added 2 commits May 18, 2025 21:39

chore(logs): standardize application logging

5bbd027

Signed-off-by: Louis Gérard <[email protected]>

docs(logs): quick note on standard log fields for an app

0352914

Signed-off-by: Louis Gérard <[email protected]>

dhawos force-pushed the chore/standardize-app-logging branch from 327e915 to 0352914 Compare May 18, 2025 19:40

crenshaw-dev approved these changes May 20, 2025

View reviewed changes

crenshaw-dev merged commit 7d58ca3 into argoproj:master May 20, 2025
27 checks passed

LyhengTep pushed a commit to LyhengTep/argo-cd that referenced this pull request May 24, 2025

chore(logs): standardize application logging (argoproj#20731) (argopr…

0478d2b

…oj#21442) Signed-off-by: Louis Gérard <[email protected]> Signed-off-by: Lyheng <[email protected]>

tylerrosnett pushed a commit to StateFarmIns/argo-cd that referenced this pull request May 27, 2025

chore(logs): standardize application logging (argoproj#20731) (argopr…

0c4ffdb

…oj#21442) Signed-off-by: Louis Gérard <[email protected]>

chansuke pushed a commit to chansuke/argo-cd that referenced this pull request Jun 4, 2025

chore(logs): standardize application logging (argoproj#20731) (argopr…

69752ff

…oj#21442) Signed-off-by: Louis Gérard <[email protected]>

dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025

chore(logs): standardize application logging (argoproj#20731) (argopr…

fde3ad9

…oj#21442) Signed-off-by: Louis Gérard <[email protected]> Signed-off-by: dsuhinin <[email protected]>

dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025

chore(logs): standardize application logging (argoproj#20731) (argopr…

bba32a4

…oj#21442) Signed-off-by: Louis Gérard <[email protected]> Signed-off-by: dsuhinin <[email protected]>

chore(logs): standardize application logging (#20731) #21442

chore(logs): standardize application logging (#20731) #21442

Uh oh!

Conversation

dhawos commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bunnyshell bot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Preview Environment undeploy from Bunnyshell failed

Uh oh!

crenshaw-dev Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

crenshaw-dev left a comment

Choose a reason for hiding this comment

Uh oh!

dhawos commented Jan 21, 2025

Uh oh!

codecov bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

crenshaw-dev commented Jan 22, 2025

Uh oh!

dhawos commented Jan 22, 2025

Uh oh!

dhawos commented Mar 9, 2025

Uh oh!

andrii-korotkov-verkada commented Mar 14, 2025

Uh oh!

andrii-korotkov-verkada commented Apr 18, 2025

Uh oh!

Uh oh!

andrii-korotkov-verkada commented May 2, 2025

Uh oh!

andrii-korotkov-verkada commented May 13, 2025

Uh oh!

dhawos commented May 16, 2025

Uh oh!

andrii-korotkov May 16, 2025

Choose a reason for hiding this comment

Uh oh!

dhawos May 18, 2025

Choose a reason for hiding this comment

Uh oh!

andrii-korotkov May 16, 2025

Choose a reason for hiding this comment

Uh oh!

dhawos May 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dhawos commented Jan 9, 2025 •

edited

Loading

bunnyshell bot commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading