Skip to content
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.

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

Fix OLAF endless logging #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix OLAF endless logging #29

wants to merge 1 commit into from

Conversation

ryanpdx
Copy link
Member

@ryanpdx ryanpdx commented Dec 16, 2024

Only keep the last 500 logs in RAM for the REST API to use.

fixes #25

@ryanpdx ryanpdx requested a review from ThirteenFish December 16, 2024 00:36
@ThirteenFish
Copy link
Contributor

A list is an inefficient datatype to use for this kind of operation, you'll want to use a deque. Not only will it be more efficient, if you:

  • set maxlen you don't have to worry about manually trimming it to 500 elements.
  • use appendleft you don't have to manually reverse it when you use it.

Copy link
Contributor

@ThirteenFish ThirteenFish left a comment

Choose a reason for hiding this comment

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

Aside from the specific changes mentioned above and below, what's stopping us from pulling the logs directly from journalctl instead of maintaining our own parallel logs? journalctl --unit=oresat-c3d --boot=0 --lines=500 --reverse does the same as what this is doing.

@@ -47,12 +55,6 @@ def on_loop(self):
self.sleep(0.1)

def on_read_since_boot(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

This callback should be part of LogServiceHandler and not LogsService. Beyond doing something involving logs they share no common interests. As an added bonus when this callback is part of LogServiceHandler, _logs no longer needs to be global.


return ret
return "\n".join(reversed(_logs))
Copy link
Contributor

Choose a reason for hiding this comment

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

_logs is accessed from multiple threads and therefore needs an associated lock to access it to be thread safe. If this callback becomes part of LogServiceHandler it can reuse the Handler's existing lock which is held when .emit() is called.


def __init__(self, **kwargs):
super().__init__(**kwargs)
self.data = []
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume self.data was left over from trying to make this work without a global _logs? If on_read_since_boot is moved to this class then this can stay, otherwise the whole overridden __init__() can be removed. (I'd obviously prefer the former.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

OLAF /tmp boot logs grow endlessly
2 participants