Expose CircuitBreaker current condition (or state) to the metrics recorder interface and Prometheus. - #25
Expose CircuitBreaker current condition (or state) to the metrics recorder interface and Prometheus.#25mickdwyer wants to merge 8 commits into
Conversation
… so the metrics record closure
slok
left a comment
slok
left a comment
There was a problem hiding this comment.
Hi @mickdwyer thanks for the contribution.
I think that this metric is valuable, adding to this I've written you some comments on the implementation, but the most important thing is how the metric is designed, having the user to "interpret" what state is the gauge value is difficult, a better approach that I've seen in some exporters and apps (e.g. kube-metrics with kube_pod_status_phase) would be to have the state on a label and setting the active state with a 1 and the others to 0.
What do you think?
| return stateStrings[st] | ||
| } | ||
|
|
||
| func (st state) condition () int { |
There was a problem hiding this comment.
I think that we could remove the integer usage for state and use only the type and the string representation of it. It would simplify the usage and we remove code.
There was a problem hiding this comment.
I agree, will refactor accordingly. (Am I right in that you're happy with the use of iota?
| } | ||
|
|
||
| func (st state) label () string { | ||
| return stateStrings[st] |
There was a problem hiding this comment.
2 things:
-
Instead of
labelwhat do you think about implementing the Stringer interface? -
This could panic if a state does not have an associated string in the array, although we control this it could lead to a runtime panic in a change. A better approach could be using a
switchwith a fallback string, would be safer.
There was a problem hiding this comment.
I'll have to read up on the Stringer interface but it feels a more natural fit.
I agree using a switch offers a more defensive implementation.
Will consider both when refactoring.
| stateOpen state = "open" | ||
| stateHalfOpen state = "halfopen" | ||
| stateClosed state = "closed" | ||
| stateNew state = iota |
There was a problem hiding this comment.
New state is the same as a closed state because a new circuitbreaker starts in a closed state. Do you have a use case where you need to know the difference between new and closed?
There was a problem hiding this comment.
My choice to implement stateNew and the transition to stateClosed on first traversal provided a mechanism to ensure that the state of the breaker was recorded on first use.
Otherwise, I found that there are no state metrics recorded until the first state change occurs.
I might take a few moments to review how I'm using the circuitbreaker instance by my application.
| // IncCircuitbreakerState increments the number of state change. | ||
| IncCircuitbreakerState(state string) | ||
| // SetCircuitbreakerCurrentCondition sets the condition of ciruit breaker. | ||
| SetCircuitbreakerCurrentCondition(condition int) |
There was a problem hiding this comment.
Would be better to have uniformity between method signatures:
- On name:
SetCircuitbreakerCurrentCondition->SetCircuitbreakerCurrentState - On parameter:
condition int->state string
There was a problem hiding this comment.
Will make the name change.
I suspect the parameter change will fall out of the refactoring of the metric approach. Stay tuned!
| switch state { | ||
| case stateNew: | ||
| // Close the breaker as this is the first time through and generate a statistic. | ||
| c.moveState(stateClosed, metricsRec) |
There was a problem hiding this comment.
As stated before, the initial state of a circuit breaker is closed, so the above change of state: stateNew, and this moveState would not be required.
There was a problem hiding this comment.
True, although it's coupled to my comment above.
| bulkProcessed *prometheus.CounterVec | ||
| bulkTimeouts *prometheus.CounterVec | ||
| cbStateChanges *prometheus.CounterVec | ||
| cbCurrentCondition *prometheus.GaugeVec |
There was a problem hiding this comment.
Uniformity naming: cbCurrentCondition -> cbCurrentState
| Help: "Total number of state changes made by the circuit breaker runner.", | ||
| }, []string{"id", "state"}) | ||
|
|
||
| p.cbCurrentCondition = prometheus.NewGaugeVec(prometheus.GaugeOpts{ |
| p.cbStateChanges.WithLabelValues(p.id, state).Inc() | ||
| } | ||
|
|
||
| func (p prometheusRec) SetCircuitbreakerCurrentCondition(condition int) { |
There was a problem hiding this comment.
The problem with this metric approach is that the user needs to be aware of what is the number when it queries, this is not very natural and could lead to easy misinterpretation of the metrics.
To have the state of a resource in a metric what I have seen as a standardized way is that the state is in a label and when one of the states is set to "active" is set to 1 and the others to 0. Is the same approach as the state counter but with a gauge of (0,1).
What do you think?
There was a problem hiding this comment.
Yes, I think that your proposal to follow the conventions you have seen elsewhere is worth adopting. I'll make the necessary refactoring to implement this design.
|
Hi @slok, thank you for the very constructive feed back. I found it particularly helpful as this is my first use of Golang. I'll comment again once I've recoded to the feedback above. |
2 participants
No description provided.