ops/log.py: Default to DEBUG logging. #236
Conversation
Pass DEBUG logging through to juju-log, and default to DEBUG level. That way users can chose something different at runtime, if they actually want/don't want to see DEBUG messages. This goes along with juju/juju#11476 which changes Juju itself to default to not displaying DEBUG messages from units.
|
I could be convinced to set the default log level to INFO, but really I think this is cleaner. Even without the Juju patch, it still provides a way for users to decide if they want DEBUG or not. In the current situation, someone has to edit the charm code to enable DEBUG logging, which is pretty difficult to do. |
| logger.debug('debug') | ||
| calls = self.backend.calls(clear=True) | ||
| self.assertEqual(len(calls), 0) | ||
| self.assertEqual(calls, res) |
There was a problem hiding this comment.
yeah, len() is rarely what you want, because you can't tell what is wrong when it doesn't match.
| model_backend -- a ModelBackend to use for juju-log | ||
| debug -- if True, enable DEBUG level logging, and write logs to stderr as well as to juju-log. | ||
| debug -- if True, write logs to stderr as well as to juju-log. |
There was a problem hiding this comment.
This sounds reasonable in general. Two questions:
-
Should we check the juju version and not spam people unless we are in a version that has fixed logs not to send debugging to people, so that we're not holding ourselves back from writing down debug statements in the code to prevent showing them to the general user?
-
Is there a context in which we don't want to log debug when running outside juju? I guess in tests it makes sense as well. Does it make sense when debugging in the shell? I guess it does as well. Any others?
There was a problem hiding this comment.
- I don't think so. Juju has always had the ability to filter late with "juju debug-log --level=INFO", so if things are spamming, there are ways to not see it. Reactive itself is very chatty at DEBUG level (see bug #1874000 for examples). So people will already be used to filtering.
Juju has also always supported people applying record-time filtering with 'juju model-config logging-config unit=INFO'. It is just that the default log level for unit messages was DEBUG.
So I think anyone bothered by verbosity already has knobs to handle it, and it is simpler and more obvious if we leave it to the existing tools. Trying to second guess them means when they really do want DEBUG, they'll find it hard to enable.
- We have bug #1861987 tracking the desire to have the juju model config for a unit exposed to the charm, so that they can chose to update the default logging. (So we don't have to exec a command if it is going to ignore the result.) In the short term, it isn't a correctness thing, just an efficiency one.
I would be happy for us to have something like an env var (juju uses JUJU_LOGGING_CONFIG) or a standard commandline parameter for main.main(). But both of those feel like a stretch in search of a use case right now, rather than a concrete need.
There was a problem hiding this comment.
wee, thanks!
if the commit message includes something like fixes: #198 github'll close the issue automatically on merge :-)
I'm slightly bothered (again and still) by our diversity of notations around args, but that's not something to fix here. I'll just mention it so you don't think I didn't notice 😆
Pass DEBUG logging through to juju-log, and default to DEBUG level. That
way users can chose something different at runtime, if they actually
want/don't want to see DEBUG messages.
This goes along with juju/juju#11476 which
changes Juju itself to default to not displaying DEBUG messages from
units.
This should address issue #198.