-
Notifications
You must be signed in to change notification settings - Fork 18
Log reason for service stop #1245
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
base: v1.x.x
Are you sure you want to change the base?
Log reason for service stop #1245
Conversation
8ac5193
to
a6e5a7b
Compare
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.
Pull Request Overview
This PR adds logging of cancellation reasons for both the background service and actor components.
- Adds a module-level logger and debug log in
_background_service.py
- Adds an info-level log in
_actor.py
when an actor is cancelled
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/frequenz/sdk/actor/_background_service.py | Imported logging , initialized _logger , and added a debug log call in cancel() |
src/frequenz/sdk/actor/_actor.py | Added an info log call in cancel() , but missing logger import/definition |
Comments suppressed due to low confidence (3)
src/frequenz/sdk/actor/_background_service.py:174
- This new debug logging for cancellation isn't covered by existing tests. Consider adding unit tests to verify that the cancellation reason is logged correctly.
_logger.debug("Service %s cancelled, reason: %s", self.name, msg)
src/frequenz/sdk/actor/_actor.py:147
- Add tests to ensure the actor's cancellation reason is logged at info level, matching the new behavior introduced here.
_logger.info("Actor %s cancelled, reason: %s", self.name, msg)
src/frequenz/sdk/actor/_actor.py:147
- The
_logger
is used here but not defined or imported. Please addimport logging
and initialize_logger = logging.getLogger(__name__)
at the top of this file.
_logger.info("Actor %s cancelled, reason: %s", self.name, msg)
52c78a0
to
f7efde7
Compare
src/frequenz/sdk/actor/_actor.py
Outdated
@@ -153,5 +153,7 @@ def cancel(self, msg: str | None = None) -> None: | |||
Args: | |||
msg: The message to be passed to the tasks being cancelled. | |||
""" | |||
if msg is not None: |
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.
Isn't this calling the super method and then its logged twice?
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.
In Actor has info log.
In Backgroud has debug log.
Having info log in BackgroundService will create lots of unnecessary logs (that we probably don't need).
Do you have different idea?
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.
Ah, well in this case it seems fine then. Didn't notice the different levels
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.
Cancel sounds like a rare event, and it's useful to know. INFO doesn't trigger any alerts. It wouldn't be too bad if we just had INFO in the background service.
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.
Oh, I didn't see this comment. For me this still sounds like a debug, because I guess it is only useful when debugging, when things are working you should see an INFO for the actor stopping, and that should be enough. That said, I'm OK to merge if there is enough consensus.
f7efde7
to
0fb0b03
Compare
Log `msg` argument to simplify debugging. Signed-off-by: Elzbieta Kotulska <[email protected]>
0fb0b03
to
1796206
Compare
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.
I'm not sure if this should be info or debug also, it seems like it might be too fine grained detail for a normal run, as we are already logging when it actually stops, so cancel would only be interesting if there is a bug and the cancel is not resulting in a stop.
@@ -167,6 +170,8 @@ def cancel(self, msg: str | None = None) -> None: | |||
Args: | |||
msg: The message to be passed to the tasks being cancelled. | |||
""" | |||
_logger.info("Service %s cancelled, reason: %s", self.name, msg) |
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.
Maybe it would be a bit easier to the eyes like this?
_logger.info("Service %s cancelled, reason: %s", self.name, msg) | |
_logger.info("Service %s cancelled%s", self.name, msg, f": {msg}" if msg is not None else "") |
To simplify debugging