Add configurable latency thresholds for persistence health#11073
Conversation
d1312db to
02f208f
Compare
| if dynamicconfig.PersistenceHealthSignalMetricsEnabled.Get(dynamicCollection)() { | ||
| aggregator := persistence.NewHealthSignalAggregator( | ||
| dynamicconfig.PersistenceHealthSignalAggregationEnabled.Get(dynamicCollection)(), | ||
| dynamicconfig.PersistenceHealthSignalPercentilesEnabled.Get(dynamicCollection)(), |
There was a problem hiding this comment.
this also slightly concerns me, because we loose the "dynamic" in the dynamic config. it can only ever change if we do a restart. where as this one keeps the dynamic config.
But I also understand that checking this every time could probably lead to problems though... just want to call out
There was a problem hiding this comment.
Luckily this is only for turning it off/on. The health checking / output side I think is dynamic?
There was a problem hiding this comment.
Maybe another good comparison is the window size and count. If we wanted those to be properly dynamic we would probably need to do some actual engineering 😬
There was a problem hiding this comment.
Thinking on this, I think we should actually have the off/on-ness be dynamic, just in case. I think we can just pass the func reference here, right?
There was a problem hiding this comment.
Yeah thats what I meant, having the dynamic-ness for the writing side. but I wonder how much that would slow things down. it would almost be like something you'd have to do async like check back every x seconds to see if dynamic config changed
There was a problem hiding this comment.
If we're lucky, the DC access function already has caching built-in? We'll have to read it to make sure though
There was a problem hiding this comment.
We already do DC access in the other health checkers path, so it seems likely its fine here for on/off? Im inclined to just do on/off though to not add a bunch of complexity to the window library.
There was a problem hiding this comment.
Added dynamicness for on/off 👍
02f208f to
02d0d65
Compare
This adds configurable latency thresholds for persistence health checks so that we can dynamically change what percentiles we want to look at, as well as what values we want to alert on. This also adds enforceability as a concept to the health checks so that we can have them reported in a safer way when testing. These latencies are computed using the same tdigest based library we use in history health checks, which will allow us to get much more useful percentile values than the current average only reporting.
02d0d65 to
42af4fd
Compare
3 participants
What changed?
This adds configurable latency thresholds for persistence health checks so that we can dynamically change what percentiles we want to look at, as well as what values we want to alert on.
This also adds enforceability as a concept to the health checks so that we can have them reported in a safer way when testing.
These latencies are computed using the same tdigest based library we use in history health checks, which will allow us to get much more useful percentile values than the current average only reporting.
Why?
So we can test out different percentile settings and evaluate good defaults.
How did you test it?
Potential risks
Changing health checks is always risky, but this adds a softer version of them so it should make things safer ultimately.