Add application name to appcontroller logs #18113
Comments
andrii-korotkov-verkada
changed the title
|
I'd be very excited to see this PR. |
|
Turns out the problem is more complicated than I thought. While some logs in appcontroller don't have info about the application, a lot of them include it in the structured data, including the example I've shown. However, when viewing the logs in the Datadog, application's fully qualified name is not available in the parsed event attributes and also not added to the log line (as it probably shouldn't). There's also been an example of logs where fields are added to the log message but in a way that indicate some broken parsing. So the source of the issue lies in ArgoCD not using JSON format recommended by Datadog for integration, and the custom Grok parser for the ArgoCD logs we have seems to not cover some edge cases (probably / in the unquoted value of the field). I'd open a discussion about potentially switching to JSON log format or finding other ways to have parsing working better. |
|
We should probably consider switching to JSON by default in 3.0. But the option is already available today. |
|
Oh, my search for potential usage of JSONFormatter was too naive. After your pointer, I've traced that to |
|
I'd just run the controller with the corresponding option, though still plan to submit the PR for some logging improvements. |
Add application fully qualified name as a logrus field to several places that were missing it. Remove appNamespace field and replace application name with a fully qualified name in a few places for consistency. Add app project to the log fields. Signed-off-by: Andrii Korotkov <137232734+andrii-korotkov-verkada@users.noreply.github.com>
Add application fully qualified name as a logrus field to several places that were missing it. Remove appNamespace field and replace application name with a fully qualified name in a few places for consistency. Add app project to the log fields. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
|
The PR is ready and passes CI. I'm not 100% sure though whether to log an app fully qualified name, or log a regular name and namespace separately. There's no consistency now - audit logger and couple of places choose the later, while multiple other places choose the former. In my PR I gave preference to the qualified name, but didn't go as far as editing audit logger. What do you think? |
|
Thinking more about this, I'll probably switch to using a regular app name and have namespace as a separate parameter, as well as separate field for a qualified name. It's going to be reasonable when querying structured data via Datadog and also supporting just using grep on raw logs. |
…ied name, add a common function for app log entry - Closes[argoproj#18113] Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
* chore: Improve appcontroller logs further - Closes [#18113] Add application fully qualified name as a logrus field to several places that were missing it. Remove appNamespace field and replace application name with a fully qualified name in a few places for consistency. Add app project to the log fields. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> * chore: Switch to using separate field for application name and qualified name, add a common function for app log entry - Closes[#18113] Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> --------- Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
|
This should be in the next release. |
…rgoproj#18174) * chore: Improve appcontroller logs further - Closes [argoproj#18113] Add application fully qualified name as a logrus field to several places that were missing it. Remove appNamespace field and replace application name with a fully qualified name in a few places for consistency. Add app project to the log fields. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> * chore: Switch to using separate field for application name and qualified name, add a common function for app log entry - Closes[argoproj#18113] Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> --------- Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
…rgoproj#18174) * chore: Improve appcontroller logs further - Closes [argoproj#18113] Add application fully qualified name as a logrus field to several places that were missing it. Remove appNamespace field and replace application name with a fully qualified name in a few places for consistency. Add app project to the log fields. Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> * chore: Switch to using separate field for application name and qualified name, add a common function for app log entry - Closes[argoproj#18113] Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> --------- Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
Summary
It would be great to have the application name mentioned in various logs of
controller/appcontroller.go. Right now it's present in some, e.g. warningFailed to store cached manifests of previous revision for app '<app>': cache: key is missing, but is missing from many others, e.g. warningSkipping auto-sync: failed previous sync attempt to <hash>.Motivation
The app controller has logs for all the apps, so a log without an app name makes it nearly impossible to do something about it. Having an app name mentioned in the logs would be a good step forward.
Proposal
Add the app name to the logs where it's missing in
controller/appcontroller.go, at least for errors and warnings. A lot of methods in the app controller code already have an app object or app name as a parameter, so it shouldn't be a hard change.The text was updated successfully, but these errors were encountered: