Combine various print result log events with different levels #6174
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
|
@jtcohen6 I added you as a reviewer to make sure that we're on the same page here, with collapsing the multiple events into one. |
|
@gshank I took a quick look through, and the consolidations proposed here make sense to me 👍 If I understand right, events will no longer reflect their log levels by inheriting from level classes ( |
|
We do lose some of the mypy type checking, since the generated proto python classes all have defaults, so if a field is not supplied on the constructor, it doesn't cause an error. That would be something that would only be an issue during development, and if the generated logging event was not checked for validity. And there's really not much that can be done about it, since it's inherent in the way that protobuf works. This particular pull request just overrides the level for the changed logging events. The rest of the logging events remain the same, in that they are based on the various level classes. |
There was a problem hiding this comment.
Is the plan to add EventInfo onto all proto events? It's hard to tell why it is on some and not others.
Example: Why does LogFreshnessResult set the level when the message is generated in freshness.py but also have a level in the definition in types.py? Or LogHookStartLine has EventInfo on the proto definition but info is not passed in.
| kind: Under the Hood | ||
| body: Combine certain logging events with different levels | ||
| time: 2022-10-28T11:03:44.887836-04:00 | ||
| custom: | ||
| Author: gshank | ||
| Issue: "6173" | ||
| PR: "6174" |
There was a problem hiding this comment.
General question: are we going to do small changelogs like this or should we consider one big one of "hey, we made big updates to structured logs to be more awesome"?
There was a problem hiding this comment.
I think it depends on the ticket. This one actually changes the codes in the logs, so if anybody were checking them, it makes sense to flag it in the changelog
|
All proto events should have EventInfo. The "level" in EventInfo is normally set with the base Level class, but the ones in this PR are an exception. We normally don't need to pass in "info" if the event only has one log level and it doesn't change. |
|
It is true that if we always pass in the level with the "info" function, we wouldn't need to use a Level base class. I just left it there just in case, but if you think we should remove it... |
I think that an event should either have a level or be dynamic. And if it's dynamic it should not use a Level base class to make that clear. |
|
I removed the Level base classes from the classes that specify the level. |
gshank
force-pushed
the
ct-1444-node_info_event_cleanup
branch
2 times, most recently
from
68f4377
to
4f4026d
Compare
gshank
force-pushed
the
ct-1444-node_info_event_cleanup
branch
from
4f4026d
to
d9319d9
Compare
| @@ -1962,177 +1940,126 @@ def message(self) -> str: | |||
|
|
|||
|
|
|||
| @dataclass | |||
| class PrintModelResultLine(InfoLevel, pt.PrintModelResultLine): | |||
| class LogModelResult(DynamicLevel, pt.LogModelResult): | |||
There was a problem hiding this comment.
Love how clear this makes it now.
resolves #6173
Description
To simplify the work that consumers of our logs need to do, combine logging events issued at the same point in the code which differ mainly in the "level".
Part of the structured logging initiative. More to follow in #5663.
Checklist
changie newto create a changelog entry