Utility for collecting new gauge metrics#9017
Conversation
ncabatoff
left a comment
ncabatoff
left a comment
There was a problem hiding this comment.
Very nice PR with great comments and tests. I have a bunch of minor suggestions, none of which are blockers.
| // so that collection processes do not all run at the time time. | ||
| // If we knew all the procsses in advance, we could just schedule them | ||
| // evenly, but a new one could be added per secret engine. | ||
| func (p *GaugeCollectionProcess) delayStart() bool { |
There was a problem hiding this comment.
Maybe rename to delayOrStop to hint at how to interpret its return value?
| } | ||
|
|
||
| if err != nil { | ||
| p.logger.Error("error collecting gauge", "id", p.labels, "error", err) |
There was a problem hiding this comment.
I worry a little about this log message being too spammy. Do we want to use lower severity? Throttle? Replace with an error metric?
Personally regardless of whether we change the logging, I like having error metrics for my metrics collections. That way when creating a dashboard or alert rule, I can easily take into account if the metrics are dubious due to a collection error.
There was a problem hiding this comment.
I added an error count.
I think the frequency of error messages will not be too high to worry about, in this case, but it might become a concern if there is a storage outage. I suspect there will be many more error messages in that case anyway?
| s.tickerBarrier = make(chan *SimulatedTicker, n) | ||
| } | ||
|
|
||
| func startSimulatedTime() *SimulatedTime { |
There was a problem hiding this comment.
This looks like a constructor, why not call it newSimulatedTime?
Co-authored-by: ncabatoff <ncabatoff@hashicorp.com>
* Name change for constructor * Allow cancel while streaming stats. * Make Done a function call rather than a public data member.
…ashicorp#9017) (hashicorp#9019) Signed-off-by: Ryan Cragun <me@ryan.ec> Co-authored-by: Ryan Cragun <me@ryan.ec>
2 participants
This module takes care of: