Logging improvements

[shsms]

No description provided.

[shsms]

I will add more commits here

[shsms]

I hope no one thinks this is overkill.

[llucax]

I don't think it is overkill but I'm not convinced about the balance. It seems to me to be too generic for being specific and too specific for being generic.

As a generic solution, I like more the approach from https://github.com/samuller/log-rate-limit, it looks super flexible, allowing even to solve the reset() issue by allowing the next N logs:

# Override the allow_next_n value for a set of logs in the same stream so that this group of logs don't restrict one
# another from occuring consecutively
logger.warning("Test", extra=RateLimit(stream_id="stream2", allow_next_n=2))
logger.info("Extra", extra=RateLimit(stream_id="stream2"))
logger.debug("Info", extra=RateLimit(stream_id="stream2"))

And if we want to go more specific, I think it would just keep track of when logs are emitted in the metric fetcher itself, so we can even print more meaningful info, like including when was the last data received ("No data received for component %d since %s.").

[llucax]

Any reason not to inherit from Logger so this can be used in places where a plain Logger is expected? If you do so you probably only need to implement log().

And maybe this could be implemented as a Filter instead (here is an example filter to de-duplicate messages), but not sure, because filters are applied at the Handler level, so it would apply to everything that's logged or we need to customize the config in such a way that messages that we want to rate-limit must be handled by a different handler.

This looks quite interesting: https://github.com/samuller/log-rate-limit, it is a filter but allows overriding on every log call too, and allows grouping messages in streams, and individual streams can be rate-limited individually.

[llucax]

I would probably just put the literal timedelta() in the constructor to avoid the indirection in the docs. If you keep it this way, you should documented via a docstring so it appears in the docs and users can know what's the default.

[llucax]

Why not just making it public?

Suggested change

	self._rate_limit: timedelta = rate_limit
	def set_rate_limit(self, rate_limit: timedelta) -> None:
	"""Set the rate limit for the logger.
	Args:
	rate_limit: Time interval between two log messages.
	"""
	self._rate_limit = rate_limit
	self.rate_limit: timedelta = rate_limit
	"""The rate limit for the logger."""

[llucax]

Suggested change

	def is_limiting(self) -> bool:
	@property
	def is_limiting(self) -> bool:

[llucax]

Why the check? It can't be None, right?

[llucax]

Couldn't you just log using the regular _logger only reset() here?

Suggested change

	_missing_data_logger.reset()
	_missing_data_logger.debug(
	"Component %d has started sending data.", self._component_id
	)
	_missing_data_logger.reset()
	__logger.debug(
	"Component %d has started sending data.", self._component_id
	)
	_missing_data_logger.reset()

[llucax]

I would rephrase because when it is repeated, one might think that it just stopped sending data now. Also since we have instances where the problem was in the data pipeline and not the component itself, maybe is more accurate to say we are not receiving data.

Suggested change

	"Component %d stopped sending data.", self._component_id
	"No data received for component %d.", self._component_id

[llucax]

This can be removed, marshmallow-code/marshmallow#2739 is fixed.