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

Make logging code self-contained #13785

Merged
merged 5 commits into from
Sep 17, 2024
Merged

Conversation

gtsiam
Copy link
Contributor

@gtsiam gtsiam commented Sep 17, 2024

  • Rewrite logging code to use python's builting QueueListener, effectively moving the logging process into a thread of the Frigate app.
  • Also, wrap this behaviour in a easy-to-use context manager to encourage some level of consistency for other processes.

@NickM-27 @hawkeye217 @blakeblackshear Sorry for pinging randomly, but I'm seeing my other PR (#13765) get forgotten, so I'm assuming it got buried by the dependabot noise.

TL;DR: Whom should I annoy to get things merged? Maintaining multiple branches gets old, fast. Unless you prefer huge, unreviewable, PRs.

I'm planning to deploy frigate in my own nvr, but I have quite a few fixes I want to contribute to get it working the way I want to. I figured you'd prefer contributions to me nagging you via issues.

Next is probably faster shutdown, assuming I don't get side tracked.

I'm also planning to improve the webui, to get it into a state where I can use that as a birdseye view with a simple web client. Is there somewhere I can reach you directly (matrix/discord/irc/pigeon)?

Rewrite logging code to use python's builting QueueListener, effectively
moving the logging process into a thread of the Frigate app.

Also, wrap this behaviour in a easy-to-use context manager to encourage
some consistency.
Copy link

netlify bot commented Sep 17, 2024

Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit fa73c86
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/66e97beb474784000853dcc2

@blakeblackshear
Copy link
Owner

We definitely prefer contributions over issues. We have had several large PRs opened in the past that haven't been merged because there was context that the author wasn't aware of. I definitely encourage you to communicate your plans prior to investing a lot of effort as we are moving relatively fast.

A series of smaller PRs like this are preferred. We are all on the Home Assistant discord.

blakeblackshear
nick_mowen_the_lawn
hawkeye217

@gtsiam gtsiam changed the title Make logging code self-contained. Make logging code self-contained Sep 17, 2024
@blakeblackshear
Copy link
Owner

The logging implementation here was based on the recommended approach for multiprocessing logging in the Python docs. https://docs.python.org/3/howto/logging-cookbook.html#logging-to-a-single-file-from-multiple-processes

I'm guessing you are suggesting we switch to the variant where the logging is a thread of the main process.

I'm curious what problem you are solving by doing this? We have quite a few things happening in the main thread when users have a large number of cameras.

@gtsiam
Copy link
Contributor Author

gtsiam commented Sep 17, 2024

Oh; I didn't realize that was in the cookbook. Still, I find that particular implementation rather dubious. Mainly in the cleanup department.

The main point of the PR is the log_thread context manager: You can just do @log_thread() or with log_thread(): to use it, instead of having to join a process in the FrigateApp in various places.

I could move log_thread to use a separate process, but:

  • QueueListener is in the standard library. The less code we have to write, the better.
  • I don't see moving to a thread from a process as neither a win nor a loss. Besides if your bottleneck is the logger, you're doing something wrong (And currently, not enough things are logged for that to be the case).
  • Fewer processes generally means easier lifecycle management (So maybe a minor win).

@gtsiam
Copy link
Contributor Author

gtsiam commented Sep 17, 2024

Oh, I forgot to run mypy. Let me fix that.

Copy link
Sponsor Collaborator

@NickM-27 NickM-27 left a comment

Choose a reason for hiding this comment

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

This PR re-introduces an issue that was fixed already which is that the onvif package forcefully introduces its own log handler which will repeat all logs without the formatting applied. This is why the

    if root.hasHandlers():
        root.handlers.clear()

is there. This can't be controlled from the onvif package

frigate/log.py Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
frigate/log.py Outdated Show resolved Hide resolved
@NickM-27
Copy link
Sponsor Collaborator

When testing inside the dev container the main process does not stop cleanly, it has to be killed

@gtsiam
Copy link
Contributor Author

gtsiam commented Sep 17, 2024

When testing inside the dev container the main process does not stop cleanly, it has to be killed.

I couldn't figure out how you're running it inside the dev container, so I'm just doing make run each time. What's the command that doesn't exit?

@NickM-27
Copy link
Sponsor Collaborator

What's the command that doesn't exit?

the main python process that runs, python3 -m frigate

frigate/__main__.py Outdated Show resolved Hide resolved
Copy link
Sponsor Collaborator

@NickM-27 NickM-27 left a comment

Choose a reason for hiding this comment

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

Okay, everything looks to be working correctly now

@hawkeye217 hawkeye217 merged commit 1c24f00 into blakeblackshear:dev Sep 17, 2024
10 checks passed
@gtsiam
Copy link
Contributor Author

gtsiam commented Sep 17, 2024

When testing inside the dev container the main process does not stop cleanly, it has to be killed.

I don't thing this is due to my changes. This seems to happen when FrigateApp crashes because of an exception and the relevant stop events are simply never raised. So non-daemon threads are never stopped.

@gtsiam gtsiam deleted the logging branch September 17, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants