Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: v1.x.x
Choose a base branch
from
Open

Log reason for service stop #1245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/frequenz/sdk/actor/_actor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,5 +153,8 @@ 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:
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

@llucax llucax Jul 17, 2025 •
edited
Loading

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.

_logger.info("Actor %s cancelled, reason: %s", self, msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this came back somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actor has info log because this log is important to see workflow. Log is printed only if msg is not None, to avoid log duplication (actor prints that it is cancelled in run method - and this log was useful many times.)

Background service has debug log, because info would log many misleading logs during normal runtime, which might be confusing. And those logs are only useful during debugging.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't put the if, I think it might be more confusing, as one might expect seeing a log entry for all cancelled operations once you saw one in the logs, you would have to be very involved in the code to know it is only logged if there is a message.

I feel pretty strongly about this one as I think it could do real harm, so for me it is either always or never.

Then again, I would prefer not to have a special case in actor with info, but I can live with that one.


self._is_cancelled = True
return super().cancel(msg)
7 changes: 7 additions & 0 deletions src/frequenz/sdk/actor/_background_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@
import abc
import asyncio
import collections.abc
import logging
from types import TracebackType
from typing import Any, Self

_logger = logging.getLogger(__name__)


class BackgroundService(abc.ABC):
"""A background service that can be started and stopped.
Expand Down Expand Up @@ -167,6 +170,10 @@ def cancel(self, msg: str | None = None) -> None:
Args:
msg: The message to be passed to the tasks being cancelled.
"""
_logger.debug(
"Service %s cancelled%s", self, f": {msg}" if msg is not None else ""
)

for task in self._tasks:
task.cancel(msg)

Expand Down
Loading