Introduce fail/pass check thresholds to prevent flapping

[edevil]

Adds two new config variables, passing_threshold and critical_threshold,
that respectively configure the number of failed checks needed to go
from healthy to critical and the number of passed checks to go from
critical to healthy.

Addresses #50

[hashicorp-cla]

All committers have signed the CLA.

[yurkeen]

Left a few notes, but otherwise great work!

[edevil]

I addressed all comments. What do you think @lornasong ?

[lornasong]

@edevil - thanks for your work here! This is really exciting! To keep you updated, I am planning to switch gears and take a look at this tomorrow (Friday) and Monday.

@yurkeen & @cbroglie thanks for engaging and reviewing!

[lornasong]

@edevil thanks again for your work! I think this is looking good.

I left two specific comments (see below). Would you be able add/update some tests that would be great for ESM's maintainability:

• updating TestDecodeMergeConfig to include the two new configs
• testing to ensure the threshold is being respected and reset

Please let me know if you have any questions. Thanks!

[edevil]

@lornasong I've updated TestDecodeMergeConfig to include the new config variables. However, I'm unsure of how to test the state of the counters since this is not exposed to the test suite.

[lornasong]

@edevil - thanks for your work! TestDecodeMergeConfig looks good. I left a few questions around clarification, decrement logic, and uint.

As for testing the counters, here's one way:

• You could use a set up similar to TestCheck_MinimumInterval in check_test.go where you can set up: test server, api client, and new check runner, and create a new check
• Then register the check in a similar way like

checks := api.HealthChecks{check}
runner.UpdateChecks(checks)

• Then retrieve the check originalCheck, ok := runner.checks[checkHash(check)] and do any checks
• Then "update" the check and confirm any values and repeat. ex:

id := checkHash(check)
runner.UpdateCheck(id, api.HealthCritical, "")
assert.Equal(t, 1, originalCheck.failuresCounter)
assert.Equal(t, 0, originalCheck.successCounter)
assert.Equal(t, api.HealthPassing, originalCheck.Status)
...

[edevil]

Hello @lornasong. I added a test case that exercises the new code.

[lornasong]

Thanks for adding the test @edevil! It helped me better understand the expected behavior of your feature. It looks really good. I left a follow up on the uint vs. int discussion

[lornasong]

Hi @edevil, thanks for the updates! I have a couple of minor suggestions to the IntValue code, apologies to not realize sooner:

• Remove Set() and String() - it looks like we aren't using them yet
• Make IntValue and Merge() private
• Update the merge() method receiver from u to i

I was also reviewing hashicorp/consul#5739 again and realized that the behavior here in ESM is different from Consul. As I understand, in ESM, the configs are for the number of additional checks, consecutive or non-consecutive, needed before switching status. In Consul, the configs are the number of total checks, consecutive only, needed before switching status.

I have some hesitation have deviant behavior from Consul since users might not expect it. You mentioned earlier that the ESM behavior is more what people will want in production and you gave a great use-case where ESM’s behavior is better suited. If you (or anyone reading) have any additional thoughts, let me know. In the meantime, I will check-in with our team early next week to get their input.

[edevil]

I've implemented the changes requested. The test failures are due to general flakiness of the test suite which I attempted to improve in #80.

[eikenb]

I restarted the test suite a couple times and they failed each time. I see #80 passed, maybe re-test this after #80 is merged to be sure it fixes the failures here?

[lornasong]

@edevil thanks for the changes and putting up #80 to fix the test flakiness! This is awesome. I am planning to review #80 tomorrow.

As an update regarding the difference in implementation between Consul and ESM: I reached out to our team and scheduled to discuss it mid next-week. I will update as soon as I learn more.

Let me know if you have any questions. Thanks!

[lornasong]

@edevil I spoke with our team. We discussed that your implementation captures more use cases than the Consul implementation and that it would be ok for ESM to diverge from Consul. Practitioners may jump to the incorrect conclusion that ESM has the same implementation as Consul since it has the same configuration name. To avoid this, let's choose a new, different configuration name.

Do you (or anyone else reading) have any thoughts on a new config name that would capture this feature? I'll include some initial ideas but we don't have to use them:

• counts_before_pass / counts_before_critical (could also use 'until' or 'to' instead of 'before)
• threshold_to_pass / threshold_to_critical
• additional_counts_to_pass / additional_counts_to_critical

Other ideas welcome!

[edevil]

I'm not very good with naming things but I would vote for threshold_to_pass. It's sufficiently different but still captures the gist of the feature.

[yurkeen]

I'd suggest passing_threshold / critical_threshold.

[lornasong]

Let's go with passing_threshold/critical_threshold. Thanks for the suggestion @yurkeen !

[edevil]

Changed the names accordingly.

[yurkeen]

Amazing work André! Thank you! Hope @lornasong can approve quickly 🙏.

[lornasong]

@edevil - this is looking great! Thanks for renaming the configurations. I added a couple final suggestions that are documentation and comments. Feel free modify as you see fit or let me know if anything is inaccurate. Thanks!

[edevil]

Thank you for the documentation help.

[lornasong]

This looks great @edevil! Thank you for all of your work here and all of the discussion! I think the community will really benefit from this feature.

[rbjorklin]

I would love to use this feature, any chance we could have a 0.5.0 release soon?

EDIT: I think a note in the CHANGELOG for this would be prudent.

[lornasong]

@rbjorklin, thanks for the question. I did another round of updates to the CHANGELOG, so you should be able to see this feature in there now. Regarding the 0.5.0 release, we have some work scheduled this week, so I am planning to release early next week. Let me know if you have any concerns. Thank you!