home-assistant / plugin-audio

Pulseaudio implementation for Home Assistant
Apache License 2.0
26 stars 13 forks source link

Remove verbose logging for pulseaudio #85

Closed tylerszabo closed 2 years ago

tylerszabo commented 3 years ago

Verbose logging can be useful for debugging but it accumulates in syslog and can fill disks.

pvizeli commented 3 years ago

we use journald which prevent you from disk filling. I don't see an issue on a supported system

tylerszabo commented 3 years ago

Unfortunately, it seems that /var/log/user and /var/log/syslog can still fill up despite using the journald log driver.

> cat /etc/docker/daemon.json
{
  "tlsverify": true,
  "tlscacert": "/etc/docker/ca.pem",
  "tlscert": "/etc/docker/server-cert.pem",
  "tlskey": "/etc/docker/server-key.pem",
  "hosts": ["fd://", "tcp://0.0.0.0:2376"],
  "log-driver": "journald"
}
tylerszabo commented 3 years ago

I dug into the issue and it seems that it's easy for rsyslog to be accidentally installed as a suggested dependency when hardening a Debian host system; even one that was initially set up as a managed OS installation (at the time I was using Hassio but this seems to be new naming).

In either case I don't think that such verbose logging should be enabled by default and even when journald is alone in limiting file size this will still compete for finite log space.

JJFourie commented 2 years ago

@pvizeli To summarize the issues related to the hassio_audio container:

  1. It runs PulseAudio with the -vvv parameter by default, i.e. with full verbose logging.
  2. hassio_audio does not support the Home Assistant standard to be able to set the log level of components in the HA Logger. And also not from the command line using a CLI like it is possible for HA Superviser (e.g. ha supervisor options -l warning).
  3. All hassio_audio logging, including the debug and info level messages, are logged as errors (level 3). Docker does support writing to stdout and stderr as different log levels in journald, as confirmed/proved [here].(https://github.com/home-assistant/plugin-audio/issues/57#issuecomment-801784738)

Another issue, not directly related to this topic, is to make it configurable to disable PulseAudio altogether. The hassio_audio component/container is compulsory, but many users are not using audio in their HA setups at all. See next point.

I'd argue that most HA users are running their implementations on low-end hardware with limited system resources, typically a Raspberry Pi with SD cards or perhaps SSD's as storage. These devices have finite write cycles, meaning at some point they will die, and unnecessary log spamming does not help at all. Filling up disks is one thing, avoiding wasting system resources and reducing write cycles is a completely different topic.

agners commented 2 years ago

I actually also prefer lowering the verboseness of the audio plug in. It is annoying when reading journal without an unit filter.

cociweb commented 2 years ago

~10 Gb daily without system logrotate on Host OS. What is missing for the Merge @pvizeli? It's a very annoying glitch, Isn't it?

pvizeli commented 2 years ago

That's the wrong way, It need to respect the logging value from supervisor like DNS pugin. If they respect it, it's fine

cociweb commented 2 years ago

That's the wrong way, It need to respect the logging value from supervisor like DNS pugin. If they respect it, it's fine

@pvizeli: Can you share more details about your thoughts? The application is started with wrong (not default) hardcoded parameter as -vvv. the pulse use default log level as notice in /etc/pulse/daemon.conf which should be adjustable. See relevant < default > section for logging:

; log-target = auto
; log-level = notice
; log-meta = no
; log-time = no
; log-backtrace = 0

If you specify the -vvv -in my thoughts- it's impossible to control it from configfile (Eg set verbosity for errors only). I think, it's independent from supervisor, Can you please share your conflicting informations, if you have?

PS: @tylerszabo Sorry, if the tone of the original message gave a shadowed meaning. It was not a goal.

tylerszabo commented 2 years ago

Hi @cociweb, I see that you're both passionate about this as I am and have a fair bit of context on how PulseAudio is behaving. Unfortunately some of the comments you're making are reading to me as being a bit hostile. I'm confident that hostility is not you're intent and that you indeed respect the often thankless effort that is maintaining an open source project. I want to make sure that we're working with @pvizeli to make the job of owning this plugin easier: to that end I think you're notes are very insightful but would benefit from some editing in the tone of the message.

I suspect that given the information in this threat that the PR here might not be sufficient but I hope that this can serve as a platform for a more robust change. Keeping the status quo is often the easier move (even when faced with a bug) and thus it's important that this discussion not become a burden for those maintaining the project.

agners commented 2 years ago

@cociweb what @pvizeli meant it should be tied to the setting of Supervisor. E.g. you can check the current log level of the Supervisor with ha info, or change to warnings using ha su options --logging warning.

The DNS plug-in respects that setting via configuration file, see here: https://github.com/home-assistant/supervisor/blob/2021.10.8/supervisor/plugins/dns.py#L257

Instead of changing the hard-coded value, the Audio plug-in should do something similar.

pvizeli commented 2 years ago

close by https://github.com/home-assistant/plugin-audio/pull/112