AnalogJ / scrutiny

Hard Drive S.M.A.R.T Monitoring, Historical Trends & Real World Failure Thresholds
MIT License
5.24k stars 166 forks source link

Webhooks & notification system #25

Closed AnalogJ closed 4 years ago

AnalogJ commented 4 years ago
AnalogJ commented 4 years ago

@bergernetch commented 27 days ago:


I still like email as a notification system.

However, for a lot of stuff I use MQTT which in turn uses my home assistant telegram integration to bring push messages to my phone..

tommyalatalo commented 4 years ago

Looking forward to Pushover implementation, would be very useful :)

AnalogJ commented 4 years ago

Hey @tommyalatalo

I have a beta version of the notifications available via this Docker image: analogj/scrutiny:notifications

Would you be willing to test it out for me?


I updated the branch README with some additional instructions -- basically a link to the shoutrrr docs since that's what I use under the hood: https://containrrr.dev/shoutrrr/services/overview/

The notifications test endpoint will return a "success" value, and any error messages in the json body.

tommyalatalo commented 4 years ago

Hey @tommyalatalo

I have a beta version of the notifications available via this Docker image: analogj/scrutiny:notifications

Would you be willing to test it out for me?

I updated the branch README with some additional instructions -- basically a link to the shoutrrr docs since that's what I use under the hood: https://containrrr.dev/shoutrrr/services/overview/

The notifications test endpoint will return a "success" value, and any error messages in the json body.

I tried setting the notifications beta image up and sent a POST request to /api/health/notify, the service fails with a certificate error when doing this (not sure if this was the test you wanted me to do though).

The error after the request is received by scrutiny:

time="2020-10-04T10:09:44Z" level=info msg="Sending notifications to pushover://shoutrrr:api@user/?devices=mi8se?priority=1&&title=Scrutiny"
time="2020-10-04T10:09:48Z" level=error msg="One or more errors occurred  occurred while sending notifications for pushover://shoutrrr:api@user/?devices=mi8se?priority=1&&title=Scrutiny:"
time="2020-10-04T10:09:48Z" level=error msg="failed to send notifications to pushover devices: [Post \"https://api.pushover.net/1/messages.json\": x509: certificate signed by unknown authority]"
time="2020-10-04T10:09:48Z" level=error msg="One or more notifications failed to send successfully. See logs for more information."
time="2020-10-04T10:09:48Z" level=error msg="An error occurred while sending test notification failed to send notifications to pushover devices: [Post \"https://api.pushover.net/1/messages.json\": x509: certificate signed by unknown authority]"
time="2020-10-04T10:09:48Z" level=error msg="192.168.1.110 - dce78d6076e2 [04/Oct/2020:10:09:48 +0000] \"POST /api/health/notify\" 500 179 \"\" \"curl/7.72.0\" (4175ms)" clientIP=192.168.1.110 hostname=dce78d6076e2 latency=4175 method=POST path=/api/health/notify referer= respLength=179 statusCode=500 userAgent=curl/7.72.0

The tokens used for the notifications should probably be readable from environment variables such as SCRUTINY_PUSHOVER_API_TOKEN and SCRUTINY_PUSHOVER_USER_KEY, so that you don't have to risk committing them into git. Like put them into a .env file which you load with docker-compose along other config, and gitignore the .env file.

A completely unrelated nitpick I would like to point out is that the service only looks for "/scrutiny/config/scrutiny.yaml" with the "yaml" extension. It would make a lot of sense to look for ".yml" as well since the short version of the extension is as common, if not more so, as ".yaml".

AnalogJ commented 4 years ago

Weird that you got a certificate error. I used pushover for testing myself, and I didn't have any issues. Though, you do have a malformed url:

pushover://shoutrrr:api@user/?devices=mi8se?priority=1&&title=Scrutiny

should probably be:

pushover://shoutrrr:api@user/?devices=mi8se&priority=1&title=Scrutiny

Regarding the environmental variables: yes, I will be supporting config from environmental variables once this notification work is complete. tracked in #52

That's a good point about /scrutiny/config/scrutiny.yaml I'll create another issue to track that.

tommyalatalo commented 4 years ago

I removed the duplicate & in the url, the certificate error remains though. Don't know if the problem its in your code or shoutrrr.

I send pushover notifications to my phone using the same credentials every day with a go project of my own, but I use a specific pushover library, not shoutrrr for that. I have received multiple notifications from my own app today, so it's likely not a problem with pushover itself.

AnalogJ commented 4 years ago

hm. So I took a look at my pushover url and compared it to yours

here's mine:

    - "pushover://shoutrrr:MY_API_KEY@MY_USER_KEY?priority=1&devices="

I don't provide the title, and I don't have a trailing slash after the user key.

Can you try that with your setup?

tommyalatalo commented 4 years ago

Using this line

- "pushover://shoutrrr:apitoken@userkey?priority=1?devices="

I get the error

time="2020-10-04T19:39:14Z" level=error msg="An error occurred while sending notifications pushover://shoutrrr:api@user?priority=1?devices=mi8se: error initializing router services: strconv.ParseInt: parsing \"1?devices=\": invalid syntax"

The same error occurs when setting the device to "mi8se" in my case. If I change the order of the devices and priority fields, and then also try defining the device the error is again the x509 unknown certificate authority.

According to the shoutrrr docs there should be a slash after the user key.

AnalogJ commented 4 years ago

double ? are not valid in the URI. Basically ? denotes the beginning of the query string, and all subsequent key/value pairs are joined with a &. Can you change your config line to:

- "pushover://shoutrrr:apitoken@userkey?priority=1&devices="

I just tested it myself, and confirmed it was working.

AnalogJ commented 4 years ago

ok I lied. I was able to replicate your issue. I've been testing with the binary directly on my dev machine. I just tried it in the container and got the certificate error. Give me a couple of minutes to figure this out.

tommyalatalo commented 4 years ago

double ? are not valid in the URI. Basically ? denotes the beginning of the query string, and all subsequent key/value pairs are joined with a &.

Obviously my brain is not working at full capacity. Drop a note here when you have a new build and I'll give it a go tomorrow.

AnalogJ commented 4 years ago

Alright, I updated the latest analogj/scrutiny:notifications image, and verified that it works for me locally. Can you try it out when you get a chance?

tommyalatalo commented 4 years ago

I pulled the new image and tried it now, sending the notification works. However there is still an error message logged in the output before logging "successfully sent notifications":

time="2020-10-05T06:47:29Z" level=info msg="Sending notifications to pushover://shoutrrr:api@user/?devices=mi8se&priority=1"
time="2020-10-05T06:47:34Z" level=error msg="One or more errors occurred  occurred while sending notifications for pushover://shoutrrr:api@user/?devices=mi8se&priority=1:"
time="2020-10-05T06:47:34Z" level=info msg="Successfully sent notifications. Check logs for more information."
time="2020-10-05T06:47:34Z" level=info msg="192.168.1.110 - 732d352a4e15 [05/Oct/2020:06:47:34 +0000] \"POST /api/health/notify\" 200 42 \"\" \"curl/7.72.0\" (5111ms)" clientIP=192.168.1.110 hostname=732d352a4e15 latency=5111 method=POST path=/api/health/notify referer= respLength=42 statusCode=200 userAgent=curl/7.72.0
AnalogJ commented 4 years ago

Great! that error message can be ignored. I'll fix that before I merge the branch.