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

Log reconnects #351

Closed
lippserd opened this issue Aug 15, 2021 · 2 comments · Fixed by #369
Closed

Log reconnects #351

lippserd opened this issue Aug 15, 2021 · 2 comments · Fixed by #369
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@lippserd
Copy link
Member

At the moment, Icinga DB only logs that it will retry reconnecting to the database (and Redis?). I think it makes sense to also log reconnects then. We could add a struct for optional args to the retry.WithBackoff() function which provides settings for OnError and OnRetry callbacks. The timeout should be moved there as well then.

@lippserd lippserd added the enhancement New feature or request label Aug 15, 2021
@lippserd lippserd added this to the v1.0.0-rc2 milestone Aug 15, 2021
@Al2Klimov
Copy link
Member

  1. I.e. not only the first error, but all/more?
  2. Why a struct?
  3. Why those settings? The callbacks already handle errors and retries by themselves. (They log the first error and silence the retries.)

@Al2Klimov Al2Klimov assigned lippserd and unassigned Al2Klimov Sep 22, 2021
@Al2Klimov Al2Klimov added the needs-feedback We'll only proceed once we hear from you again label Sep 22, 2021
@lippserd
Copy link
Member Author

The idea with the struct and the callbacks is that the retry package is the only place that handles the error / retry conditions and not the consumers of that package. In the logs I would expect something like this:

Can't connect to database / Lost connection to database. Reason. Retrying
...
Reconnected to database after 1 minute 6 seconds.
...

For the sake of completeness, the OnRetryableError callback could have a variable that indicates whether it is the first error after the (re) connection.

@lippserd lippserd assigned Al2Klimov and unassigned lippserd Sep 22, 2021
@lippserd lippserd removed the needs-feedback We'll only proceed once we hear from you again label Sep 22, 2021
Al2Klimov added a commit that referenced this issue Sep 22, 2021
Al2Klimov added a commit that referenced this issue Sep 22, 2021
Al2Klimov added a commit that referenced this issue Sep 22, 2021
E.g. the first "connection refused" and the first "hostname mismatch".

refs #351
Al2Klimov added a commit that referenced this issue Sep 22, 2021
... to give the admin the all-clear.

refs #351
Al2Klimov added a commit that referenced this issue Sep 23, 2021
... to give the admin the all-clear.

refs #351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants