switch to http client for pushover notifier#1490
Conversation
crazy-max
force-pushed
the
pushover-http
branch
from
534b3b3 to
cf2edc7
Compare
| req.Header.Set("User-Agent", c.meta.UserAgent) | ||
|
|
||
| resp, err := hc.Do(req) | ||
| if err != nil { |
There was a problem hiding this comment.
@crazy-max why not check the status code of the response here? According to the docs, a non-200 response code will not generate an error. If the user entered incorrect credentials and you get a 401/403 here, for example, there's an opportunity to provide a much better error message.
There was a problem hiding this comment.
You only ever consume/log the StatusCode in case of a failure to parse JSON, which may not fail in the case of non-success responses because the error body might be JSON formatted. This may lead to very confusing behavior for the user to diagnose.
| Token string `json:"token"` | ||
| } | ||
|
|
||
| if err = json.NewDecoder(resp.Body).Decode(&respBody); err != nil { |
There was a problem hiding this comment.
Suggest logging the full unparsed response body (e.g. as a string) when debug logging turned on. As well as basic HTTP stuff like status code.
There was a problem hiding this comment.
It is next line if unmarshal fails:
diun/internal/notif/pushover/client.go
Line 141 in a70601f
There was a problem hiding this comment.
Oh yeah I meant unconditionally log it! Not just on failure to parse as JSON.
I'm not sure if that's a convention you follow, but I find it useful to have those sorts of things in verbose logs for providing more context when diagnosing issues -- even if the call is successful. That's what the Debug log is for, IMO 🙂
There was a problem hiding this comment.
And actually... On a second look, I'm not sure the existing log statement will work as intended. It's trying to serialize the struct, which won't be populated since JSON deserialization failed. I think you need to read the body as a string and log that instead.
| appReset = time.Unix(int64(i), 0) | ||
| } | ||
| } | ||
| log.Debug().Msgf("Pushover app limit: %d, remaining: %d, reset: %s", appLimit, appRemaining, appReset) |
There was a problem hiding this comment.
You don't want to return early with an error here? Don't we already know the POST failed due to rate limiting if we get here? Won't this run the risk of the app assuming the POST was successful and recording it as such in the DB, when in reality it never went through?
I think ideally there'd be some mechanism to automatically retry the request later after appReset has elapsed. (And possibly start queuing other requests since we know they will fail until the limit is renewed.) But at the very least it should return an error so higher level code paths don't assume the post was successful.
Having that many images need an update at the same time, such that the rate limit is exhausted, might be somewhat of an edge case or only a first-run concern... But IMO it just speaks to the reliability & "completeness" of the Pushover integration!
There was a problem hiding this comment.
Ah forgive me, I misread this if block. This is just gathering the information, which might be there even on successful calls.
I guess the sort of thing I described above would need to live in an if checking for StatusCode == 429.
There was a problem hiding this comment.
Yes didn't want yet to bail out if rate-limiting detected but we could as follow-up. Just wanted to log it first.
| form.Add("url_title", c.meta.Name) | ||
| } | ||
| form.Add("timestamp", strconv.FormatInt(time.Now().Unix(), 10)) | ||
| form.Add("html", "1") |
There was a problem hiding this comment.
Might make sense to log the request body under Debug. (Maybe with token sanitized?)
|
Good call rolling your own Pushover client! 🧠 Definitely seems to make sense here since it's not too complex and gives you much finer control. |
2 participants
relates to:
Should help debugging
error="invalid character '<'error.cc @DrEsteban @RickDB