Log Persistence

[SteveMicroNova]

What does this change intend to accomplish?

Supports #741 by adding a setting to toggle log persistence on the configs page

Checklist

• [x] Have you tested your changes and ensured they work?
• [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• [x] If applicable, have you updated the CHANGELOG?
• [x] Does your submission pass linting & tests? You can test on localhost using ./scripts/test
• [x] If this is a UI change, have you tested it across multiple browser platforms?
• [x] If this is a UI change, have you tested across multiple viewport sizes (ie. desktop versus mobile)?

[rtertiaer]

I'm of split mind between putting this in the main amplipi setup, and in the updater (which we should probably rename "admin" or similar.)

[SteveMicroNova]

I'm of split mind between putting this in the main amplipi setup, and in the updater (which we should probably rename "admin" or similar.)

@linknum23 Thoughts?

[linknum23]

Sorry I didn't get a chance to look at this before my early weekend adventures.

Currently the only thing that modifies the logging is the updater so modifying it in amplipi as well feels awkward. However that would mean that this menu has to live in the updater/admin interface somewhere. The right choice is not obvious to me right now. That probably means we need a better interface for stuff like this.

[SteveMicroNova]

Currently the only thing that modifies the logging is the updater

Where at? I'm looking at the updater and I just see the latest update, a way to revert to previous updates, a way to upload custom updates, a way to set a password for the web interface, and the support tunnel. Nothing that has to do with logs as far as I can see

[SteveMicroNova]

Marking this as ready to review as both people that have chimed in are uncertain as to if this belongs in the updater or not If we solidly say "this belongs in the updater", it will be reverted to draft while I work on that

[SteveMicroNova]

Requesting review from Ryan as he reviews all my things

Requesting review from Lincoln as he's already had input and I would like to continue to ask his opinion, but I do not need his review per se

Requesting review from Klayton to potentially have a tiebreaker for the "should the be in the updater or not?" issue

[rtertiaer]

Where at? at a minimum here, called from the "updater side" of things.

[rtertiaer]

my vote would be to put this in the updater, given the above.

[klay2000]

I don't feel like this goes in the updater because it's not exactly related to updates, however I also don't feel like the regular Amplipi web app should be able to change something like this. Given that the updater also has the support tunnel stuff I suppose there is some precedent for something like this to go there. Ultimately I think as was said before we need a better place for this kind of thing, but for now the updater is good enough.

[rtertiaer]

imo, the "updater" has never been an updater, but an administration interface (and once upon a time, the only administration you could do was updating the appliance.) it's a separate app with elevated privileges, accessible even when the main app breaks.

[SteveMicroNova]

https://github.com/user-attachments/assets/5784bc4e-b71b-4807-a947-2f6f587e699f

Log persistence has been moved to the updater page (now called admin) and plugged in properly See video for changes and proof of function, or go to lamplipi now to see for yourself. Next steps are make sure you can actually access persisted logs from the logs page, and find where those are stored in the first place

[SteveMicroNova]

Next steps are make sure you can actually access persisted logs from the logs page, and find where those are stored in the first place

I've found the file they persist to and verified that the logs page can find them, the only thing is persistent logs always persist, and will be on top of the pile on the logs page even when you turn off persistent storage. We should garbage collect this a bit, the only question is on what basis.

My immediate ideas:

1. simply wipe them out the second log persistence is turned off
2. turn on a midnight cronjob to clear the persist logs directory of all files at midnight if persist logs is deactivated

[linknum23]

Should we change the Settings name from "Admin" to "Admin / Updates" to maintain consistency with the old interface?

[SteveMicroNova]

Revoking reviews requested and marking as draft, I've found more bugs in this PR due to working on a PR based on this one (I'm building out the admin settings tab with more options)

Will rerequest review and mark as ready for review once I fix those on this branch rather than just in my unpublished MoreAdminSettings branch I'm currently working on

[linknum23]

I like the certainty of something that will turn off after some time but I also get the need/want for full control. This really should only be a temporary solution to help diagnose users' problems.

I wonder if there is a realistic timeframe that would work for most users?

For more permanent logging on the current hardware I would suggest hooking this up to an external server or an attached USB key.

On Thu, Aug 8, 2024 at 5:59 PM Steven Engelbert @.***> wrote:

@.**** commented on this pull request.

In amplipi/updater/static/index.html https://github.com/micro-nova/AmpliPi/pull/828#discussion_r1710348152:

•            async function setPersist() {
  

•              const checkbox = document.getElementById("persist_logs_checkbox");
  

•              const response = await fetch("/settings/persist_logs", {method: "POST"});
  

•            }
  

•            window.onload = getPersist;
  

•          </script>
  

•          <div id="admin-settings-dialog">
  

•            This tab allows you to change settings relating to the core of AmpliPi, things that do not belong on the standard config page
  

•          </div>
  

•          <p class="">
  

•            <a class="btn btn-primary btn-sm text-center" id="set-password-button" data-toggle="tab" href="#set-password" role="tab" aria-controls="set-password" aria-selected="false">Set Password</a>
  

•          </p>
  

•          <h6>WARNING: leaving log persistence on for extended periods can damage your system and void your warranty</h6>
  

Instead of changing the setting automatically, would it suffice to have the frontend occasionally pop a notification that's like "hey, look out, this setting is still on!" And have it be colored yellow instead of the green/red the other notifications are

Perhaps not due to the number of people that only use a custom API caller to interact with the device, but I feel Ryan's instinct (or we least what I've interpreted his instinct to be) that we should leave this high level control in the hands of the device owners rather than trying to know better than them is for the best. Inform where necessary, but leave them to do their thing without any bothersome automations getting in the way of them doing what they want to do with their own system(s).

— Reply to this email directly, view it on GitHub https://github.com/micro-nova/AmpliPi/pull/828#discussion_r1710348152, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEZPOZ43ZU2JBBUTLKDC4TZQPS3RAVCNFSM6AAAAABLMXE4VWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMRYHEZTOMRQGI . You are receiving this because you were mentioned.Message ID: @.***>

[SteveMicroNova]

I like the certainty of something that will turn off after some time but I also get the need/want for full control. This really should only be a temporary solution to help diagnose users' problems. I wonder if there is a realistic timeframe that would work for most users?

I've elected to shoot for a middleground where there's a "persist logs auto-deactivate delay" that can be set right alongside the persist logs status, defaults to 14 days from activation with the option to set to 0 to turn off the auto-deactivation

I've worked on that all day and I'm not yet ready to commit it as it's nonfunctioning atm, but I'm announcing that to be my intended solution and it seems pretty good so far imo

[SteveMicroNova]

I've elected to shoot for a middleground where there's a "persist logs auto-deactivate delay" that can be set right alongside the persist logs status

This took some working and reworking, but I think it's done I initially had them be a separate string of endpoints, but then I noticed that the persist_logs bool and the auto_off_delay int were too closely related to keep it that way, rerouted all the logic to make them work within the same pydantic object for every endpoint and made a script and cronjob to run the script that makes the auto_off_delay actually matter. Will be self-reviewing for commentary shortly

[SteveMicroNova]

[linknum23]

Could this be stylized a bit maybe with some added spacing and more significant headers? I can't tell where one config stops and the next one starts.

[SteveMicroNova]

[linknum23]

Here's a couple of things that could make the log persistence config more cohesive

• Move the warning underneath the checkbox and make it grey?
• Change the "Log Persistence" check boxes label to "Enable" since we have the header above

[SteveMicroNova]

Here's a couple of things that could make the log persistence config more cohesive

• Move the warning underneath the checkbox and make it grey?
• Change the "Log Persistence" check boxes label to "Enable" since we have the header above

[linknum23]

I think this looks a lot better!

[SteveMicroNova]

imo, this doesn't close out System log persistence during important events #741 unless this logging is turned on before/during important events

https://github.com/micro-nova/AmpliPi/issues/741#issuecomment-2243223153 Was this not your preferred solution after we talked about it? I suppose it doesn't solve the issue to the letter, but to the spirit of the issue it adds the correct functionality

[rtertiaer]

this ticket supports #741, in that it allows us to save logs persistently. However, it does not save logs during important events like upgrading or opening a support tunnel, which is explicitly what the ticket calls out in the title and description. If that is simply infeasible for whatever reason, that's a different story, but without that justification it doesn't close that ticket imo.

[SteveMicroNova]

There is now a notification of success or failure Admittedly it's not terribly useful as it doesn't tell you what happened, but it's easy and non-intrusive. It also comes up fast enough that it should be fully visible without a shake I think, the button literally changed to say "Yea" or "Nay"

https://github.com/user-attachments/assets/e4bc840b-e7a7-4b85-a4a2-f321d169be50

[linknum23]

Looks like we are going to need to update some update docs in several places here and on the interwebz

[SteveMicroNova]

Looks like we are going to need to update some update docs in several places here and on the interwebz

Yeah, that was my concern with that change. There's a lot of places that we can't change either such as other people's posts on discourse and old email chains, though luckily those instances feel like they've got fairly limited blast radius I'll make sure to update them in my #923 branch if this is merged before that (which I expect)

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 81 lines in your changes missing coverage. Please review.

Project coverage is 50.09%. Comparing base (7499989) to head (6e5a026). Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
amplipi/updater/asgi.py	0.00%	81 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #828      +/-   ##
==========================================
- Coverage   50.67%   50.09%   -0.58%     
==========================================
  Files          40       40              
  Lines        7154     7232      +78     
==========================================
- Hits         3625     3623       -2     
- Misses       3529     3609      +80     

Flag	Coverage Δ
unittests	50.09% <0.00%> (-0.58%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.