Reset notifyAfterCount on Success

[glanchow]

See Issue statping#911

[jemand771]

hi there and thanks for you contribution! :)

I've looked at the linked issue and your change and this seems to be a valid fix for it. I didn't have time to test it yet, though.
quick follow-up: from what I can see, s.notifyAfterCount isn't used anywhere else (such as a global fail counter etc.) - in that case, line 83 seems to be redundant since the counter is already incremented above. (likely a copy/paste error, same as the one you fixed)

do you see anything against removing that aswell while we're at it?

[glanchow]

Hi, thank you for your fast answer.
I think that this notifyAfterCount line 83 is executed when NotifyAfter == 0, the previous notifyAfterCount line 61, executed when NotifyAfter != 0, is followed by a return statement.

[jemand771]

yeah, s.notifyAfterCount gets incremented in line 83 if s.NotifyAfter is 0, in which case there is no need to keep track of the count there

I could imagine somebody adding a feature that uses this counter (e.g. "resettable" notifiers) at some point, so I don't really care if we leave it in or not ^^

[glanchow]

oh yes, I misunderstood your point the first time I read it
I agree and I will remove it right away

[jemand771]

okay, my testing setup for this was really manual, but this PR seems to fix the mentioned issue 🎉

note: I feel like notifiers were wonky in general, especially the reported time (e.g. 292 years 24 weeks). not related to this PR though

[adamboutcher]

@jemand771 we good to merge this?

[jemand771]

@adamboutcher oh yeah, this is good to go ^^

as a general workflow question - should I just merge PRs that look good to me (if I feel like I'm able to judge that) by myself or assign to you or..? I feel like we haven't really talked about that yet

[adamboutcher]

For now, if you think its a good pr then why not just merge, we can always roll back. Maybe in the future we just need two reviewers (especially if someone else with go knowledge steps up)

[glanchow]

I tested this version in production for a few weeks and it works well. You can merge

[jemand771]

just as a side note, the commit that was added after my review (6a80260) slightly changed the behaviour of the code:
when a notifier fails (for some reason), the states are updated anyway. (previously, they would only be updated after all notifiers had been processed)

this way, if a notifier fails, statping won't retry notifying anymore and will instead think that it has already done that.

I don't see how a notifier would even fail temporarily, so it might not matter. (we don't want to retry for permanently broken notifiers)
just in case we see dropped notifications at some point, this might be related

[adamboutcher]

The notifiers that depends on 3rd party services could fail, such as the "Mail".

[glanchow]

yes I did this on purpose
imho a success should reset the retry counter, even if one of possible multiple notifiers is broken
if there is a need to retry notifications we should had more logic