feat: Provide an option to change the juju-log formatting#1881
feat: Provide an option to change the juju-log formatting#1881addyess wants to merge 1 commit intocanonical:mainfrom addyess:issue/1880/slightly-improved-logger
Conversation
addyess
changed the title
|
Ben is on vacation, so let's ping @tonyandrewmeyer instead. |
dimaqq
left a comment
dimaqq
left a comment
There was a problem hiding this comment.
Please describe why a different format is needed?
What kind of format is needed?
Are only DEFAULT and VERBOSE going to be used, or do you envision a very custom format?
How would a given charm decide when to toggle the format?
Do you expect every app in a deployment to use the same format, or are disparate formats good enough?
Considering all the other ops users, I feel that there needs to be a good reason to tweak the format:
- today we don't make any promised on the exact format
- with this change, we'd on the hook to support the verbose format forever?
| """A handler for sending logs and warnings to Juju via juju-log.""" | ||
|
|
||
| DEFAULT_FORMATTING = None | ||
| VERBOSE_FORMATTING = '%(name)s:%(lineno)-5d %(message)s' |
There was a problem hiding this comment.
Hot take with a dash of sarcasm: expect less pretty output at 100K LOC?
There was a problem hiding this comment.
Also docstring is needed under the public constant.
| root_logger = logging.getLogger() | ||
| for handler in root_logger.handlers: | ||
| if isinstance(handler, cls): | ||
| handler.setFormatter(logging.Formatter(format or cls.DEFAULT_FORMATTING)) |
There was a problem hiding this comment.
I'm a little confused here. DEAULT is None, so why format or DEFAULT?
Is this to escape user supplying an empty string?
There was a problem hiding this comment.
Yes, an empty string would render the logs silent. Every message would be:
- unit-my-charm-0: 13:05:35 INFO unit.my-charm/0.juju-log something the charm wishes to log
+ unit-my-charm-0: 13:05:35 INFO unit.my-charm/0.juju-log| method(message) | ||
| calls = backend.calls(clear=True) | ||
| level, message = result | ||
| expected = re.compile(rf'{logger.name}:\d+\s+{message}') |
There was a problem hiding this comment.
While I like the test, I would prefer a test vector.
Like the exact log line produced with default and verbose formats.
If that's somehow hard (e.g. time), then I'd like to see examples in a comment.
There was a problem hiding this comment.
i think i didn't do precisely that because the line-number of the call to line 108
You end up with a log message that looks something like this.
test_log:108 critical
| @classmethod | ||
| def set_formatting(cls, format: str | None = DEFAULT_FORMATTING): | ||
| """Adjust the log format output.""" |
There was a problem hiding this comment.
Better docstring is needed for a public function.
| for handler in root_logger.handlers: | ||
| if isinstance(handler, cls): | ||
| handler.setFormatter(logging.Formatter(format or cls.DEFAULT_FORMATTING)) |
There was a problem hiding this comment.
I'm not a fan of this loop. Can this be better?
Also, it seems that early logging would use the default format and only later the format would be switched. Am I reading this right?
There was a problem hiding this comment.
Also, it seems that early logging would use the default format and only later the format would be switched. Am I reading this right?
I'm not a big fan of this either, although charms could obviously switch up the formatting at any point anyway.
I think perhaps we ought to be consistently using getLogger(__name__) in the ops modules (e.g. we do in charm and model, we don't in storage), and we should have a consistent format for those, which means we'd have a handler that is specific for those. We could then leave the root logger for charms to use and they can configure the root logger's (juju-log) handler's formatter to be whatever they want.
There was a problem hiding this comment.
It's fine... at least the PR gets ideas rolling. I''m mostly just currently bummed by not having the following in my logs: %(name)s and %(lineno)s. If you want to change anything around underneath -- fine with me. It's just tough at the moment to not have these two items in my logs.
Here's a sample from the k8s charm with and without it
- unit-k8s-0: 13:05:35 INFO unit.k8s/0.juju-log k8s/0(juju-04ff10-noble-0) current cluster-name=k8s-bd08d4f998c4e658c2c4139659f824ac
- unit-k8s-0: 13:06:24 INFO unit.k8s/0.juju-log Feature 'load-balancer' enabled=False,deployed=False,ver=v0.14.9-ck0,updated_at=2025-07-01 22:01:42+00:00: disabled
+ unit-k8s-0: 13:05:35 INFO unit.k8s/0.juju-log __main__:384 k8s/0(juju-04ff10-noble-0) current cluster-name=k8s-bd08d4f998c4e658c2c4139659f824ac
+ unit-k8s-0: 13:06:24 INFO unit.k8s/0.juju-log events.update_status:128 Feature 'load-balancer' enabled=False,deployed=False,ver=v0.14.9-ck0,updated_at=2025-07-01 22:01:42+00:00: disabledThere was a problem hiding this comment.
all the stuff juju-log puts out front is nice i guess.
- I like the timestamp,
- I like the level.
I don't know why the unit name is in there two times? unit-k8s-0 and unit-k8s/0.juju-log. It feels cluttered
But ops can't change any of that...
you have control over everything past juju-log here -- lets make the most of that real-estate
|
Closed based on the comments on the issue leaning towards a more unified solution rather than quick-fix of my very mundane problem |
3 participants
Addresses #1880
Provides the charmer to adjust the formatter to something more pleasant without forcing it on others.
Usage