Skip to content
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

Update log levels #2244

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Update log levels #2244

merged 1 commit into from
Mar 6, 2023

Conversation

acosta11
Copy link
Member

@acosta11 acosta11 commented Mar 2, 2023

Is there a related GitHub Issue?

#1926

What is this change about?

Audit and update all existing logs to an appropriate level. We generally did the following to logs:

  • Use error log level for platform startup issues, debug log level for resource lifecycle events and info log level for everything else.
  • Attach an error to an info log entry as a "reason" if appropriate.
  • Deduplicated error logging

Note that after rebasing on main there is a new helper function that is logging at the error level. We left the helper alone for now and will confirm with the team whether there is any issue with moving that down to info by default, potentially with a sibling helper function.
Updated the new helper function to info log level like the rest of the controller logs returning errors back to the api.

Does this PR introduce a breaking change?

No

Acceptance Steps

Deploy Korifi normally and see many fewer noisy logs at the default log level.

Tag your pair, your PM, and/or team

@davewalter @matt-royal
cc @julian-hj for thoughts

@acosta11 acosta11 marked this pull request as draft March 2, 2023 22:14
@acosta11
Copy link
Member Author

acosta11 commented Mar 2, 2023

@clintyoshimura @Birdrock did y'all have any thoughts on the log level used by the new logErrorAndSetReadyStatus helper function? We considered moving the logs down to Info level given the additional error handling of the ready status in the api layer, but wanted to check with y'all first.

@acosta11 acosta11 force-pushed the issues/1926 branch 2 times, most recently from 59e21af to acc1da4 Compare March 2, 2023 23:32
@acosta11 acosta11 marked this pull request as ready for review March 6, 2023 18:20
- Use error log level for platform startup issues, debug log level for
  resource lifecycle events and info log level for everything else.
- Attach an error to an info log entry as a "reason" if appropriate.
- Default to info logs for errors in controllers that also set status
  condition
- Make capitalization consistent for log messages (only capitalize
  kubernetes resource names)

Co-authored-by: Matt Royal <mroyal@vmware.com>
Co-authored-by: Dave Walter <walterda@vmware.com>
Co-authored-by: Ashwin Krishna <krishnaas@vmware.com>
Copy link
Contributor

@clintyoshimura clintyoshimura left a comment

Choose a reason for hiding this comment

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

LGTM

@acosta11 acosta11 merged commit c768f83 into main Mar 6, 2023
@acosta11 acosta11 deleted the issues/1926 branch March 6, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants