WIP: Implement Prometheus statser #355
Conversation
|
Hooray! All contributors have signed the CLA. |
pkg/web/prometheus.go
Outdated
| ) | ||
|
|
||
| // TODO: now I have setup the Prometheus metric objects, how do I integrate with gostatsd internal metrics measuring | ||
| // tools? How do I run this file, and start writing some tests for this. Need to adhere to the project testing styles. |
There was a problem hiding this comment.
The expected interface is a stats.Statser (https://github.com/atlassian/gostatsd/blob/master/pkg/stats/statser.go), and you can embed a stats.flushNotifier to take care of the RegisterFlush and NotifyFlush calls.
You'll likely need to lazily create metrics on-demand and store them in a map - gostatsd doesn't have any arbitrary metrics, so you don't need to worry about removing things from the map. The map can be protected with an RWMutex, since after warmup, it will be very rare (if ever), that additional metrics are created.
There was a problem hiding this comment.
Also, there's basically no testing of the actual Statsers. There is a bit for pkg/web, but it's acceptable to just make sure you get a 200 return code when it's expected.
…d with flushNotifier
…and add prom statser type check
| } | ||
| } | ||
|
|
||
| func (ps *PrometheusStatser) NotifyFlush(ctx context.Context, d time.Duration) { |
There was a problem hiding this comment.
You don't need to implement NotifyFlush and RegisterFlush, it's picked up implicitly from the embedded flushNotifier
There was a problem hiding this comment.
How come the TaggedStatser implements those two functions or why does TaggedStatser not use a flushNotifier as one of its attributes?
There was a problem hiding this comment.
My bad, that comments from my personal account.
The TaggedStatser is a wrapper that explicitly calls to the wrapped Statser.
It shouldn't use a flushNotifier directly, because then the TaggedStatser and the underlying Statser would have different notification targets (and ultimately MetricFlusher.Run would only call NotifyFlush on the underlying Statser)
That being said, it could and probably should use an embedded Statser and remove the explicit implementation.
There was a problem hiding this comment.
Oh, I see that now, what was the design process behind, is it because that they have different notification targets, so one such as TaggedStatser is a wrapper of Statser but InternalStatser is not a wrapper because they have the same notification targets?
I am using the GoLand IDE, and I am trying to find the usage of NotifyFlush, for some reason I cannot see any call to this method outside of the statsers and the flush_notifer.
There was a problem hiding this comment.
There was no special design considerations behind TaggedStatser not embedding the Statser, it's possible at the time that I thought you could only embed a struct in a struct, and an interface in an interface, but not an interface in a struct, I don't recall for sure though.
InternalStatser does explicitly implement NotifyFlush however, as it needs to take action on the flush before returning. Flush notification is an asynchronous, best effort action, and any metrics triggered by it are processed in the next flush cycle.
The InternalStatser however explicitly needs to process the metrics in the current flush cycle, so it takes action (dispatching internal metrics) before returning.
I'm not sure why GoLand doesn't detect usage - it does in my setup. You could try rebuilding the index through File -> Invalidatae Caches / Restart, but I don't know if it will help.
The only place it's called is at https://github.com/atlassian/gostatsd/blob/master/pkg/statsd/flusher.go#L69
There was a problem hiding this comment.
My problem was I was not aware that the Statser interface also has a NotifyFlush method, I was only trying to find the usage of NotifyFlush from the flsuhNotifier struct.
There was a problem hiding this comment.
Ahhh, that would do it :)
| type PrometheusStatser struct { | ||
| flushNotifier | ||
|
|
||
| // collector of gauges that stores the internal gauge metrics of gostatsd |
There was a problem hiding this comment.
Each GaugeVec and CounterVec is a single metric name, they need to be lazily created.
There was a problem hiding this comment.
Does it make sense to use the Gauges and Counter structs gostatsd already provides or would using the structs provided from the prometheus package be better?
There was a problem hiding this comment.
You'd need to use the ones from the prometheus packages, because they're ultimately added to the prometheus registry.
|
|
||
| // TODO: how do I use tags here, same for the Count and the TimingMS methods | ||
| func (ps *PrometheusStatser) Gauge(name string, value float64, tags gostatsd.Tags) { | ||
| ps.gaugeVec.WithLabelValues(name).Add(value) |
|
Just as an aside, I believe prom uses |
Currently working on #284.