Miksus / rocketry

Modern scheduling library for Python
https://rocketry.readthedocs.io
MIT License
3.25k stars 105 forks source link

ENH Avoid mem build-up in default log storage #142

Open autoluminescent opened 1 year ago

autoluminescent commented 1 year ago

Description The use of RedBird to store logs can easily lead to unexpected memleaks, and it is both a difficult problem to debug, and a little awkward to handle that on the side of the client.

Consider the following script:

import tracemalloc
from rocketry import Rocketry
app = Rocketry(execution="main")
tracemalloc.start()
last_count, _ = tracemalloc.get_traced_memory()

@app.task("every 1 second")
def do_things() -> None:
    global last_count
    new_count, _ = tracemalloc.get_traced_memory()
    print(new_count - last_count)
    last_count = new_count

if __name__ == "__main__":
    app.run()

Which results in the following input:

145402
14344
6636
6604
6668
6604
6604
6604
6677
7290
6604

So every run causes a memory build-up of around 6.5KB even if no logging is made, other than the default Rocketry logging. In 24h that would be over 500MB.

Using tracemalloc.take_snapshot() it is easy to check that this is caused by the RedBird in-memory log store.

This is solvable, but in an arguably awkward way:

import tracemalloc

from redbird import BaseRepo
from redbird.repos import MemoryRepo
from rocketry import Rocketry
from rocketry.log.log_record import LogRecord
logger_repo = MemoryRepo(model=LogRecord)
app = Rocketry(execution="main", logger_repo=logger_repo)
tracemalloc.start()
last_count, _ = tracemalloc.get_traced_memory()

@app.task("every 1 second")
def do_things() -> None:
    global last_count
    logger_repo.filter_by().delete()
    new_count, _ = tracemalloc.get_traced_memory()
    print(new_count - last_count)
    last_count = new_count

if __name__ == "__main__":
    app.run()

The reason why I say this is awkward is that it requires:

Potential solution Ideally, Rocketry's default log repo has some mechanism to evict old records automatically, which would be configurable. So for example, if we simply did:

app = Rocketry()

We would get a logger repo that would keep, for example, the last 100 log messages only. If we wanted to keep all records, we'd just pass the basic MemoryRepo as in my example above. If we wanted to control the amount of records kept, we could do something like:

from redbird.repos import FIFOMemoryRepo
from rocketry.log.log_record import LogRecord

app = Rocketry(logger_repo=FIFOMemoryRepo(model=LogRecord, size=50))

The FIFOMemoryRepo class would have to be implemented in RedBird, of course.

Miksus commented 1 year ago

A bit background: The rational for the default logging is that it's easy to prototype and get started with Rocketry. Wasn't meant for production and the assumption was there would be some thousand records max in generic cases.

But I think you have a very valid point: there should be a hard limit by default and I think your proposal would be the way to go. I'm thinking maybe the limit could be like 1000 or so.

Thanks for the idea and I really appreciate the time you spent on investigating the issue and the solutions. I think there was another similar issue but due to lack of elaboration there I didn't get the grip on the whole issue from that.

autoluminescent commented 1 year ago

Thanks for your reaction! I'm glad my suggestion resonated with you.

As for the default limit, yes, 1000 log entries seems reasonable. It means that Rocketry's default mem requirement for a task that doesn't produce any logs by itself is capped at around 6.5 MB, which seems ok.

Let me know if you'd like some help with it in case you decide to go forward with an implementation.

Miksus commented 1 year ago

I'll do!

I'm currently working on adding optional timezone as that seems to be quite important for quite many people. After that I either try to progress on the event system or then I focus on Red Bird issues possibly including this issue. I'll notify you then if you wish to participate (like in the discussion at least, would be great to have your opinions).

vagosduke commented 10 months ago

Hello! I had this issue in production and fixed it with a LimitedMemoryRepo. Also implemented a NullRepo which does not track any logs whatsoever (is this going to break anything, e.g. if query_data does nothing?)

I can push my changes to be included in Rocketry as alternative log repos to the list offered by redbird