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.

Sign up for GitHub

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

Expose CircuitBreaker current condition (or state) to the metrics recorder interface and Prometheus. #25

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Expose CircuitBreaker current condition (or state) to the metrics recorder interface and Prometheus. #25

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b5e2dd3
WIP: expose a Recorder built from Prometheus' DefaultRegisterer
mickdwyer Mar 20, 2019
4d29958
WIP: create an unused circuit breaker state and close it on first use…
mickdwyer Mar 20, 2019
fcfae81
WIP: fix typo
mickdwyer Mar 20, 2019
57a6f52
WIP: fix typo
mickdwyer Mar 20, 2019
8978c16
WIP: rename the unused state to new
mickdwyer May 5, 2019
db7bf6a
WIP: use an itoa for the breaker state
mickdwyer May 5, 2019
2bf49fb
WIP: record the current circuit breaker state in the metrics recorder
mickdwyer May 5, 2019
c89af62
rename SetCircuitbreakerCurrentCondition to SetCircuitbreakerCurrentS…
mickdwyer May 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions circuitbreaker/circuitbreaker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,30 @@ import (
"github.com/slok/goresilience/metrics"
)

type state string
type state uint

const (
stateOpen state = "open"
stateHalfOpen state = "halfopen"
stateClosed state = "closed"
stateNew state = iota
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@mickdwyer mickdwyer May 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stateOpen
stateHalfOpen
stateClosed
)

var stateStrings = []string {
"new",
"open",
"halfopen",
"closed",
}

func (st state) label () string {
return stateStrings[st]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 things:

  • Instead of label what 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 switch with a fallback string, would be safer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func (st state) condition () int {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, will refactor accordingly. (Am I right in that you're happy with the use of iota?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iota is perfect

return int(st)
}

// Config is the configuration of the circuit breaker.
type Config struct {
// ErrorPercentThresholdToOpen is the error percent based on total execution requests
Expand Down Expand Up @@ -124,7 +140,7 @@ func NewMiddleware(cfg Config) goresilience.Middleware {

return func(next goresilience.Runner) goresilience.Runner {
return &circuitbreaker{
state: stateClosed,
state: stateNew,
recorder: newBucketWindow(cfg.MetricsSlidingWindowBucketQuantity, cfg.MetricsBucketDuration),
stateStarted: time.Now(),
cfg: cfg,
Expand Down Expand Up @@ -157,6 +173,9 @@ func (c *circuitbreaker) Run(ctx context.Context, f goresilience.Func) error {
func (c *circuitbreaker) preDecideState(metricsRec metrics.Recorder) {
state := c.getState()
switch state {
case stateNew:
// Close the breaker as this is the first time through and generate a statistic.
c.moveState(stateClosed, metricsRec)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, although it's coupled to my comment above.

case stateOpen:
// Check if the circuit has been the required time in closed. If yes then
// we move to half open state.
Expand Down Expand Up @@ -223,7 +242,8 @@ func (c *circuitbreaker) moveState(state state, metricsRec metrics.Recorder) {

// Only change if the state changed.
if c.state != state {
metricsRec.IncCircuitbreakerState(string(state))
metricsRec.IncCircuitbreakerState(state.label())
metricsRec.SetCircuitbreakerCurrentCondition(state.condition())

c.state = state
c.stateStarted = time.Now()
Expand Down
1 change: 1 addition & 0 deletions metrics/dummy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ func (dummy) IncBulkheadQueued() {}
func (dummy) IncBulkheadProcessed() {}
func (dummy) IncBulkheadTimeout() {}
func (dummy) IncCircuitbreakerState(state string) {}
func (dummy) SetCircuitbreakerCurrentCondition(condition int) {}
func (dummy) IncChaosInjectedFailure(kind string) {}
func (dummy) SetConcurrencyLimitInflightExecutions(q int) {}
func (dummy) SetConcurrencyLimitExecutingExecutions(q int) {}
Expand Down
2 changes: 2 additions & 0 deletions metrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ type Recorder interface {
IncBulkheadTimeout()
// IncCircuitbreakerState increments the number of state change.
IncCircuitbreakerState(state string)
// SetCircuitbreakerCurrentCondition sets the condition of ciruit breaker.
SetCircuitbreakerCurrentCondition(condition int)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to have uniformity between method signatures:

  • On name: SetCircuitbreakerCurrentCondition -> SetCircuitbreakerCurrentState
  • On parameter: condition int -> state string

Copy link
Author

@mickdwyer mickdwyer May 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make the name change.
I suspect the parameter change will fall out of the refactoring of the metric approach. Stay tuned!

// IncChaosInjectedFailure increments the number of times injected failure.
IncChaosInjectedFailure(kind string)
// SetConcurrencyLimitInflightExecutions sets the number of queued and executions at a given moment.
Expand Down
19 changes: 18 additions & 1 deletion metrics/prometheus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"github.com/prometheus/client_golang/prometheus"
)

var (
const (
promNamespace = "goresilience"

promCommandSubsystem = "command"
Expand All @@ -17,8 +17,11 @@ var (
promCBSubsystem = "circuitbreaker"
promChaosSubsystem = "chaos"
promConcurrencyLimitSubsystem = "concurrencylimit"

)

var DefaultPrometheusRecorder = NewPrometheusRecorder(prometheus.DefaultRegisterer)

type prometheusRec struct {
// Metrics.
cmdExecutionDuration *prometheus.HistogramVec
Expand All @@ -28,6 +31,7 @@ type prometheusRec struct {
bulkProcessed *prometheus.CounterVec
bulkTimeouts *prometheus.CounterVec
cbStateChanges *prometheus.CounterVec
cbCurrentCondition *prometheus.GaugeVec
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uniformity naming: cbCurrentCondition -> cbCurrentState

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make this change

chaosFailureInjections *prometheus.CounterVec
concurrencyLimitInflights *prometheus.GaugeVec
concurrencyLimitExecuting *prometheus.GaugeVec
Expand Down Expand Up @@ -59,6 +63,7 @@ func (p prometheusRec) WithID(id string) Recorder {
bulkProcessed: p.bulkProcessed,
bulkTimeouts: p.bulkTimeouts,
cbStateChanges: p.cbStateChanges,
cbCurrentCondition: p.cbCurrentCondition,
chaosFailureInjections: p.chaosFailureInjections,
concurrencyLimitInflights: p.concurrencyLimitInflights,
concurrencyLimitExecuting: p.concurrencyLimitExecuting,
Expand Down Expand Up @@ -122,6 +127,13 @@ func (p *prometheusRec) registerMetrics() {
Help: "Total number of state changes made by the circuit breaker runner.",
}, []string{"id", "state"})

p.cbCurrentCondition = prometheus.NewGaugeVec(prometheus.GaugeOpts{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above Condition -> State

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make this change.

Namespace: promNamespace,
Subsystem: promCBSubsystem,
Name: "current_condition",
Help: "The current condition of the circuit breaker runner.",
}, []string{"id"})

p.chaosFailureInjections = prometheus.NewCounterVec(prometheus.CounterOpts{
Namespace: promNamespace,
Subsystem: promChaosSubsystem,
Expand Down Expand Up @@ -172,6 +184,7 @@ func (p *prometheusRec) registerMetrics() {
p.bulkProcessed,
p.bulkTimeouts,
p.cbStateChanges,
p.cbCurrentCondition,
p.chaosFailureInjections,
p.concurrencyLimitInflights,
p.concurrencyLimitExecuting,
Expand Down Expand Up @@ -210,6 +223,10 @@ func (p prometheusRec) IncCircuitbreakerState(state string) {
p.cbStateChanges.WithLabelValues(p.id, state).Inc()
}

func (p prometheusRec) SetCircuitbreakerCurrentCondition(condition int) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

p.cbCurrentCondition.WithLabelValues(p.id).Set(float64(condition))
}

func (p prometheusRec) IncChaosInjectedFailure(kind string) {
p.chaosFailureInjections.WithLabelValues(p.id, kind).Inc()
}
Expand Down
15 changes: 15 additions & 0 deletions metrics/prometheus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,21 @@ func TestPrometheus(t *testing.T) {
`goresilience_circuitbreaker_state_changes_total{id="test2",state="close"} 1`,
},
},
{
name: "Recording circuitbreaker circuit breaker condition should expose the condition.",
recordMetrics: func(m metrics.Recorder) {
m1 := m.WithID("test")
m2 := m.WithID("test2")
m1.SetCircuitbreakerCurrentCondition(0) // new
m1.SetCircuitbreakerCurrentCondition(3) // open
m1.SetCircuitbreakerCurrentCondition(1) // close
m2.SetCircuitbreakerCurrentCondition(2) // half-open
},
expMetrics: []string{
`goresilience_circuitbreaker_current_condition{id="test"} 1`,
`goresilience_circuitbreaker_current_condition{id="test2"} 2`,
},
},
{
name: "Recording chaos metrics should expose the metrics.",
recordMetrics: func(m metrics.Recorder) {
Expand Down