Raft and state store indexes as metrics #5841
Conversation
nomad/server.go
Outdated
| select { | ||
| case <-time.After(period): | ||
| commitIndex := s.raft.LastIndex() | ||
| metrics.SetGauge([]string{"raft", "commitIndex"}, float32(commitIndex)) |
There was a problem hiding this comment.
Should this be logIndex instead of commitIndex? At least the comments in https://github.com/hashicorp/raft/blob/master/state.go#L66 suggest (because of "cache") that we're getting a possibly stale index from the raft max(log, snapshot). This may be fine, I'm still a little hazy on which ids from raft may move backwards.
There was a problem hiding this comment.
It looks to me like logIndex and commitIndex are distinct, and probably both helpful. The only place the commit index is returned from the raft api looks like its in the Stats call, https://github.com/hashicorp/raft/blob/master/api.go#L943
| @@ -109,6 +109,18 @@ when retrieving metrics using the above described signals. | |||
| <td>Raft transactions / `interval`</td> | |||
| <td>Counter</td> | |||
| </tr> | |||
There was a problem hiding this comment.
@langmartin @schmichael Can i get a review on these docs before I merge?
| func (s *Server) EmitRaftStats(period time.Duration, stopCh <-chan struct{}) { | ||
| for { | ||
| select { | ||
| case <-time.After(period): |
There was a problem hiding this comment.
very nit-picky - timer.Ticker is slightly nicer for using in loops, to reuse channels and timer mechanisms.
| metrics.SetGauge([]string{"raft", "appliedIndex"}, float32(appliedIndex)) | ||
| stateStoreSnapshotIndex, err := s.State().LatestIndex() | ||
| if err != nil { | ||
| s.logger.Warn("Unable to read snapshot index from statestore, metric will not be emitted", "error", err) |
There was a problem hiding this comment.
Curious - what conditions would result into an error getting the latest index? Also, is it meant to be recoverable? I'd be concerned about spurious logging if it happens somewhat frequently.
There was a problem hiding this comment.
This should be rare and indicates state store corruption (either the index table is missing or contains invalid data). A lot of Nomad operations would be broken if it gets to that state so its a good thing to be warning about here.
| </tr> | ||
| <tr> | ||
| <td>`nomad.raft.appliedIndex`</td> | ||
| <td>Index of the last applied log</td> |
There was a problem hiding this comment.
~ Is it worth adding some context and/or the caveat in https://godoc.org/github.com/hashicorp/raft#Raft.AppliedIndex :
This is generally lagging behind the last index, especially for indexes that are persisted but have not yet been considered committed by the leader. NOTE - this reflects the last index that was sent to the application's FSM over the apply channel but DOES NOT mean that the application's FSM has yet consumed it and applied it to its internal state. Thus, the application's state may lag behind this index.
nomad/server.go
Outdated
| @@ -410,6 +411,9 @@ func NewServer(config *Config, consulCatalog consul.CatalogAPI) (*Server, error) | |||
| // Emit metrics | |||
| go s.heartbeatStats() | |||
|
|
|||
| // Emit raft and state store metrics | |||
| go s.EmitRaftStats(time.Second, s.shutdownCh) | |||
There was a problem hiding this comment.
I see how this matches other stats period, but I suspect that this is too frequent. Statsd agent commonly only flush data every few seconds seconds (e.g. 10 seconds for datadog [1] and statsite [2]), and with gauges, all values in the flush interval are dropped except for the last one [3].
Nothing to change now but raising awareness of potentially wasteful operations here and we can investigate/act later.
[1] https://docs.datadoghq.com/developers/dogstatsd/#how-it-works
[2] http://statsite.github.io/statsite/
[3] https://github.com/statsd/statsd/blob/master/docs/metric_types.md#gauges
There was a problem hiding this comment.
Would it be more reasonable to use 10seconds here? The one potentially expensive call is state.LatestIndex which scans multiple memdb tables to find the max index. I'd rather fix this now to be more conservative about emitting stats.
There was a problem hiding this comment.
Yeah, I'd go with 10s or 5s, and then we can reexamine other stats periods.
| @@ -109,6 +109,18 @@ when retrieving metrics using the above described signals. | |||
| <td>Raft transactions / `interval`</td> | |||
| <td>Counter</td> | |||
| </tr> | |||
| <tr> | |||
| <td>`nomad.raft.lastIndex`</td> | |||
| <td>Index of the last log</td> | |||
There was a problem hiding this comment.
it's the index of the last log or snapshot, I assume there are cases where the snapshot can lead the logs. Since snapshot is just a view of the logs, this may not be a meaningful distinction
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR makes every server emit raft commit and apply indexes, and the state store's latest index as metrics