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

Make the log level back to warning for unclassified exc #35206

Merged
merged 1 commit into from
Aug 6, 2016

Conversation

hu-dabao
Copy link
Contributor

@hu-dabao hu-dabao commented Aug 4, 2016

What does this PR do?

Make the log level back to warning for unclassified exceptions.
The issue was made in this commit

There are three reasons why we need to change the log level back:

  1. It was at warning level, but the commit above make it to debug without explanation.
  2. It is helpful to make unclassified exceptions to warning as these exceptions would be critical.
  3. We don't need to set the log level back to debug, restart minion, rerun/reproduce the module calls, etc. , troubleshoot the issue, make the log level back to what it was again and restart salt-minion again.
  4. Especially for salt-api calls, the salt service owner may not be able to reproduce the salt-api call easily with confidence.
  5. Back to warning level will be consistent with line

What issues does this PR fix or reference?

Previous Behavior

When I see "The minion function caused an exception" in the log and I want to debug on, I have to do the following:

  1. set the log level back to debug
  2. restart minion
  3. rerun/reproduce the module calls, etc.
  4. troubleshoot the issue
  5. set the log level back to what it was
  6. restart minion

New Behavior

I just need to do one step: troubleshoot the issue.

Tests written?

No

Please review Salt's Contributing Guide for best practices.

@cachedout
Copy link
Contributor

I think this is very reasonable. What do you think, @s0undt3ch ?

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Aug 6, 2016
@@ -1389,7 +1389,7 @@ def get_executor(name):
ret['out'] = 'nested'
except Exception:
msg = 'The minion function caused an exception'
log.warning(msg, exc_info_on_loglevel=logging.DEBUG)
log.warning(msg, exc_info_on_loglevel=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of True use the log level you wish the traceback to be shown at, if I understand correctly, logging.WARNING or instead of exc_info_on_loglevel pass exc_info=True.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, and yes, this is reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the latest commit. Thanks for pointing this out.

@hu-dabao
Copy link
Contributor Author

hu-dabao commented Aug 6, 2016

@cachedout
Hi there,

I addressed @s0undt3ch 's comment by the latest commit. Should be good to merge.
Let me know if you guys have any other questions.

Thanks

@cachedout cachedout merged commit eeede82 into saltstack:2016.3 Aug 6, 2016
@cachedout
Copy link
Contributor

Thanks very much @hu-dabao !

@hu-dabao hu-dabao deleted the fix-exc-log branch August 8, 2016 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants