metrics: emit stats for vault token next_renewal & last_renewal #5222 #12435
Conversation
|
@jazzyfresh looks like |
There was a problem hiding this comment.
These are super nice!
I will ask the team about the ms vs ns thing, but regardless of the unit we need to document these new metrics in https://www.nomadproject.io/docs/operations/metrics-reference
Probably even worth mentioning them in https://www.nomadproject.io/docs/operations/metrics-reference#key-metrics.
Next token renewal should never be negative, if it does, it means that Nomad found an unrecoverable error and will not attempt to renew its Vault token anymore.
Last token renewal should never go past the Nomad token TTL, and going past TTL/2 should probably be a warning. If this happens it means that Nomad is not able to renew its token.
Token TTL is kind of a mix of them. It should never go below zero and dipping below TTL/2 should be a warning.
nomad/vault.go
Outdated
| v.currentExpiration = time.Now().Add(time.Duration(ttlSeconds) * time.Second) | ||
| now := time.Now() | ||
| v.currentExpiration = now.Add(time.Duration(ttlSeconds) * time.Second) | ||
| v.lastRenewalTime = now |
There was a problem hiding this comment.
Right now extendExpiration is only called in parseToken, which runs once when the client starts, so the metric works, but it seems like it would be easy to call it somewhere else and accidentally reset this metric.
Updating this value inside renew (once it succeeds) sounds like a better option, or maybe inside the renewalLoop inside the if err == nil block and the use the same lock for last and next renewal?
There was a problem hiding this comment.
extendExpiration is called in renew too.
There was a problem hiding this comment.
Yup, but my point is that it should only be called in renew, otherwise it will reset the metric value. You would also want to reset it whenever renew is called, so this action is tied to the renew process, not necessarily to the expiration extension.
my goland linter is broken again T-T |
|
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. |
Closes #5222