-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Native support for health check ping urls #577
Comments
Hey @timvahlbrock - thanks for the nice words. Have you seen the sync metrics capability which can be enabled through a flag. Anything missing from this that you‘d need to achieve your goals? |
Well having the data easily accessible is nice, but as far as I can tell I still need to monitor myself, whether an automated run succeed or not, right? |
Yes - the data is written to a file that you'd need to watch (I'm using telegraf to collect the data and Grafana to display and alert) Opposite to that you are looking for a webhook functionality, right? I've never designed such a feature, so implementing this will require an understanding of how much customisation it needs to allow in order to be useful. Nevertheless, feel free to propose a design, in case you have experience or a good idea :) |
Well, I would do it pretty much like borgmatic (see link above): a single additional parameter that is the URL to ping. |
Drafted together a quick PoC PR, haven't tested it yet and I would also like to include the log in the send pings. But on changes to the configuration the PR should be complete. |
@steilerDev Due to the comment you left on the webUi issue I thought about the mentioned separation of lib and cli frontend a bit and noticed that there is an inconsistency in the current health check url implementation with this. The health check pin executor is located within the app, but the url is stored within the resource manager. Without the health check ping executor, the url isn't used anywhere. So to align with the separation the url would have two be passed to the ping executor without handing it to the library. As far as I can tell this would be the first instance of a command line argument that would be processed exclusively outside of the library itself. Alternatives would be to leave it like it is or to make the health check ping part of the library. |
The thinking behind the current architecture is that the iCloud App utilizes the Library to execute and schedule syncs. The Eventconsumer's are part of the app - this includes the HealthCheck, but also the Error Reporter and Log utility (all of them read CLI parameter from the Resource manager). In my opinion, the WebUI would be another Eventconsumer that's part of the app. Or am I misunderstanding something here? |
I just found it a bit odd that the lib is responsible for storing a configuration value (the health check url) that are exclusively used by app. If someone is using just the lib part this kind of configuration values are basically dead to them. I thought that was an implementation mistake on my side, but the error handler is doing something similar with
Yes, this isn't related to the web ui a lot. It was just your comment about the separation there that made me remember the existence of the separation. |
So there probably isn't any urgent ToDo on this, I just should have investigated a bit more before writing the earlier comment. |
Describe the ultimate goal you want to achieve
Compliments/Background
Thanks for this great library, I'm finally able to create automatic backups of the 20 year old photo library of my family, will still having all the nice perks of iCloud Photos. Have it running on my K8s Homeserver, setup was pretty straightforward. Even managed to set up a process to simplify the token renewal each month.This great library supports setup as a daemon process or can alternatively be setup with other mechanisms to make it fully autonomous (apart from the monthly token refresh). It would be great to have the possibility to easily monitor the status of the automation process.
How do you think the feature should be implemented
The backup tool I use, borgmatic allows to set health check ping URLs. When the backup is done the configured URL is queried, if the health check hoster does not receive a ping, or the request is appended with a non-zero number (the exit code), the health check provider can notify the user of the missing or failed backup, for example via email. The health check ping can be enhanced with more detail, by adding the logs in the request body, which can then be included in the notification or email. This is currently still possible, but with a little more effort and tweaking. For my setup I overwrote the entry point of the container with the following commands.
While doing the health check ping manually provides a little more flexibility (for example I actually added a few more lines to include the number of downloaded photos and the consumer storage), the configuration could be a little simpler if the url can just be included in the configuration.
I would be willing to provide a pull request for this.
Checklist
The text was updated successfully, but these errors were encountered: