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

Native support for health check ping urls #577

Open
2 tasks done
timvahlbrock opened this issue Nov 16, 2024 · 9 comments
Open
2 tasks done

Native support for health check ping urls #577

timvahlbrock opened this issue Nov 16, 2024 · 9 comments
Labels
class(feature) Indicates work related to a new feature status(in progress) This item is currently in scope for the next release

Comments

@timvahlbrock
Copy link

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.

exec 2>&1;
set -o pipefail;
icloud-photos-sync sync | tee /tmp/icloud-photos-sync.log;
exitCode=$?;
curl -fsS -m 10 --retry 5 --data-binary @/tmp/icloud-photos-sync.log https://hc-ping.com/the-healthcheck-slug/$exitCode;
exit $exitCode

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

@timvahlbrock timvahlbrock added class(feature) Indicates work related to a new feature status(open) A new issue - not yet classified labels Nov 16, 2024
@steilerDev
Copy link
Owner

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?

@timvahlbrock
Copy link
Author

timvahlbrock commented Nov 17, 2024

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?

@steilerDev
Copy link
Owner

steilerDev commented Nov 17, 2024

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 :)

@timvahlbrock
Copy link
Author

Well, I would do it pretty much like borgmatic (see link above): a single additional parameter that is the URL to ping.

@timvahlbrock
Copy link
Author

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.

@timvahlbrock timvahlbrock changed the title Frist level support for health check ping urls Native support for health check ping urls Nov 17, 2024
@steilerDev steilerDev added status(in progress) This item is currently in scope for the next release and removed status(open) A new issue - not yet classified labels Feb 13, 2025
@timvahlbrock
Copy link
Author

@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.

@steilerDev
Copy link
Owner

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?

@timvahlbrock
Copy link
Author

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).

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 enableCrashReporting.

In my opinion, the WebUI would be another Eventconsumer that's part of the app.

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.

@timvahlbrock
Copy link
Author

So there probably isn't any urgent ToDo on this, I just should have investigated a bit more before writing the earlier comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class(feature) Indicates work related to a new feature status(in progress) This item is currently in scope for the next release
Projects
None yet
Development

No branches or pull requests

2 participants