Conversation
|
|
||
| def __init__(self, interval: float, capacity: int = LOKI_MAX_BATCH_BUFFER_SIZE, **kwargs): | ||
| super().__init__(capacity, **kwargs) | ||
| self.interval = interval |
There was a problem hiding this comment.
I see that in LokiQueueHandler the batch_interval is optional but when it gets propagated to LokiQueueHandler we don't consider interval as being optional. Maybe I'm missing something. My worries are related to (time.time() - self._last_flush_time >= self.interval) condition used in shouldFlush fn because we'll get a TypeError if self.interval is None.
There was a problem hiding this comment.
python-logging-loki/logging_loki/handlers.py
Lines 26 to 29 in c200531
In the LokiQueueHandler constructor there is check wether or not batch_interval is set? This should be sufficient, right?
There was a problem hiding this comment.
Now that I see this chunky code i'm gonna change it to use a ternary operator
There was a problem hiding this comment.
Probably too late for this library, but I would (here and elsewhere) only use dependency injection in the__init__ methods, and not create objects from properties (like the target handler in this case) .. the init should only be a factory method for the class itself, not its members ...
You can then still have other static builder/factory methods that create the objects and stitches them together.
There was a problem hiding this comment.
I would love to add dependency injection as well, but since this repo is a fork, I just want to make minimal changes
| self._last_flush_time = time.time() | ||
|
|
||
| def flush(self) -> None: | ||
| self.acquire() |
There was a problem hiding this comment.
does this acquired lock imply that all logs made during the flush are lost because of the
if not self._lock.acquire(blocking=False):
return
in __call__?
There was a problem hiding this comment.
NVM, it's a lock on another object ... (the emitter, not the handler)
There was a problem hiding this comment.
Indeed, the handler lock is to prevent the entire buffer of logs to be flushed/sent twice
The other lock used in the emitter is to prevent the emitter from creating an infinite loop:
when it's forwarding a log line to loki, we don't want to forward any newly created log lines in the emit methods itself (e.g. from urrlib/httpx/requests because of the request to loki)
|
Had a late response too #10 (comment) |
3 participants
lokiEmitterV0