Log when no relevant samples are found for resampling #923
Conversation
5a29f4d to
29c1732
Compare
|
This is a very useful debugging metric and would have pointed us to the underlying issue in the location with None values much earlier. |
| _logger.warning( | ||
| "No relevant samples found for component: %s", | ||
| component_name, | ||
| ) |
There was a problem hiding this comment.
@shsms should I keep the log as a warning or this should be just a debugging log?
There was a problem hiding this comment.
I think warning makes sense, because somebody is expecting data that's not showing up. But it would also lead to a lot of noise when a component is under maintenance, etc. I wonder if we can just log once when an outage starts and once when it ends
There was a problem hiding this comment.
This is a much more general issue, this applies to a lot of issues. Eventually I think we really need an alerting mechanism, not sure if the notifications service will be used for that. Looking at logs should be a very low level thing, users should just get notified when something went wrong.
Services like sentry (and probably any observability service out there) can set alarms when a particular type of warning (or log message) if firing for the first time, or if it is firing a number of times per second, or per hour, etc. and you can silence alerts also for some amount of times (when stuff is in maintenance for example).
At some point we'll need to have this kind of flexibility. For now I think warning every time is a considerable improvement. If it is trivial to do the log when it starts and when it ends, we could add it too, but I wouldn't do it if it makes the code much more complicated.
29c1732 to
6b774d9
Compare
| @@ -745,6 +745,8 @@ def resample(self, timestamp: datetime) -> Sample[Quantity]: | |||
| # So if we need more performance beyond this point, we probably need to | |||
| # resort to some C (or similar) implementation. | |||
| relevant_samples = list(itertools.islice(self._buffer, min_index, max_index)) | |||
| if not relevant_samples: | |||
| _logger.warning("No relevant samples found for component: %s", self._name) | |||
There was a problem hiding this comment.
Maybe it could be interesting to log the contents of the self._buffer here too? Just to know which samples where the last received and when?
There was a problem hiding this comment.
I think logging self._buffer could indeed be useful for debugging the resampler itself. However, for operational purposes, only the component name is necessary, as information can be obtained from the component logs and querying the current state of the component is enough.
In our specific operational scenario, we didn't find a need for accessing self._buffer. Should I still add a _logger.debug() for self._buffer?
|
I added this to rc6, as I guess we want to ship it ASAP. |
Yes, please. |
Haha, do it then! You can either change the target branch of this PR to v0.25.x and we then merge v0.25.3 to into v1.x.x, or you create a separate PR for the v0.25.x branch. |
I'll create a separate PR for the v0.25.x branch once this is approved/merged |
These additional logs will help in understanding why certain microgrid components, such as meters, fail to provide relevant samples for resampling. Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
6b774d9 to
463efec
Compare
To add an entry in the `Enhancements` session related to the new warning message that is logged when no relevant samples are found in a component for resampling. Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
463efec to
922cb31
Compare
|
Only updated the release-notes. |
These additional logs will help in understanding why certain microgrid components, such as meters, fail to provide relevant samples for resampling.