-
Notifications
You must be signed in to change notification settings - Fork 63
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
Reduce Logging noise #678
base: main
Are you sure you want to change the base?
Reduce Logging noise #678
Conversation
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.
Hey Mark, this is awesome job! Left a few comments and thoughts
@ChristianZaccaria Tested this out and it seems no matter the log level it will always print logs below its level. e.g. set log level to 6 you will get all logs below level 6 too. I have opted to make warning logs like More informational logs that are noisy have been bumped to log level 4 |
/retest Seems the CI is acting up. I'm pretty sure it has nothing to do with your changes. |
Hey Mark, could you try change i.e., a comment or add a comment and push changes, just to trigger the CI again and see if it works. The |
/retest |
1 similar comment
/retest |
314c7dc
to
b189216
Compare
Hi @Bobbins228, I've ran MCAD locally with the debugger, and when I attempt to bring up a cluster object (cluster.up), it initially says there is insufficient resources to dispatch appwrapper. Then, I get the Raw of a GenericTemplate which are lots of numbers such as below: |
@ChristianZaccaria What log level do you have your MCAD set to? |
On the debugger it's set to 15 by default, I thought that could be it but, update on my previous comment: With this PR I don't see the raw GenericTemplate on MCAD logs when ran in the cluster. However, when I run mcad (main branch) via the debugger, I don't get the Raw GenericTemplate. Something in this PR is causing the debugger to display that for some reason. |
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.
/lgtm /approve logs are displayed as expected. Verified with log level set to 6. Great job Mark!
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.
Thanks, can you please address the review
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.
Great job! Thanks for addressing all the requested changes. /lgtm
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ChristianZaccaria The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue link
Closes #612
What changes have been made
Removed occurrences where an entire appwrapper is logged
Bumped large informational logs up to level 4 and kept warning logs as level independent.
Added new log which logs additional resources required for an appwrapper to be dispatched
Verification steps
Set the logging level for MCAD to be level 3 and check that level 3 related logs are accurate.
Repeat with level 4 logs.
To verify the required resources log
Create an AppWrapper that has a large number of resources e.g. 8 GPUs you should receive a log like this:
Checks