Initial Kafka output metrics #162
Conversation
|
Changes Unknown when pulling bc04a80 on kfcOutMetric into ** on master**. |
|
Changes Unknown when pulling b3cd954 on kfcOutMetric into ** on master**. |
| @@ -72,3 +71,4 @@ import: | |||
| - package: github.com/Shopify/sarama | |||
| version: ^1.11.0 | |||
| repo: http://github.com/Shopify/sarama | |||
| - package: github.com/rcrowley/go-metrics | |||
There was a problem hiding this comment.
Can you give a short summary of:
- the value this new dependency brings in (that we don't already have)
- why is that important to us
- why is this the right way, going forward, say compared to https://github.com/uber-go/tally
Unless there is a strong reason to pull in this dependency, I am not inclined to go down this road.
uber-go/tally is close to what we already have and provides the missing features on top of it.
There was a problem hiding this comment.
The strong reason is that the Kafka client library depends on this to provide metrics, hence we don't get a choice. Either this, or no Kafka metrics.
common/metrics/go-metrics-m3.go
Outdated
| closeCh <-chan struct{}, | ||
| ) gometrics.Registry { | ||
| // Make a new registy and give it to the caller. This allows the exporters to operate independently | ||
| registry := gometrics.NewRegistry() |
There was a problem hiding this comment.
I think it would be cleaner to actually pass in the 'gometrics.Registry' as an argument to "NewGoMetricsExporter" .. leaving it to the caller to do 'gometrics.NewRegistry', etc.
The purpose of this code should only be an "exporter", in that you give it the 'gometrics.Registry' and the metrics map .. and it would export them to m3.
There was a problem hiding this comment.
I might also argue, this should not be part of the "common/metrics" package .. but I'll leave it to you if you want to reconsider moving this (gometrics-exporter) out as a separate module, etc.
There was a problem hiding this comment.
The common/metrics package thing might make sense. Let's talk in person.
There was a problem hiding this comment.
Looks like making it a package also means that I have to make a new directory, which will contain just this one file.
common/metrics/go-metrics-m3.go
Outdated
| // goMetricsExporter allows an rcrowley/go-metrics registry to be | ||
| // periodically exported to the Cherami metrics system | ||
| type goMetricsExporter struct { | ||
| c Client |
There was a problem hiding this comment.
Since this is a "metric" client .. I think 'm' (or perhaps m3) would be more appropriate than 'c':
r.c.AddCounter(r.scope, metricID, count-lastVal)
common/metrics/go-metrics-m3.go
Outdated
| // Calculate the time to provide the delayToFirstLogDump in export() | ||
| lastLogDump: time.Now().Add(-1 * delayBetweenLogDumps).Add(delayToFirstLogDump), | ||
| } | ||
| go r.run() |
There was a problem hiding this comment.
We tend to use the (New, Start, Stop) pattern in a lot of 'modules' in our code. You might want to consider that .. where 'Start' does a 'go r.run()' and 'Stop' closes the (internal) closeCh, etc.
There was a problem hiding this comment.
I'm trying to keep this quite simple on the calling side, i.e. just an init and it handles itself, i.e. magic. I can see the argument in following an established pattern, but that seems like it just adds a few more lines of code on the caller side, and it's speculative (i.e. it presumes that the caller would want to stop the metric reporting at some time other than the natural end of its life). Let's talk in-person.
common/metrics/go-metrics-m3.go
Outdated
| } | ||
|
|
||
| func (r *goMetricsExporter) export() { | ||
| r.registry.Each(func(name string, i interface{}) { |
There was a problem hiding this comment.
I am not sure I understand this. So for every metric in the 'go-metrics' registry, we emit into m3? This would mean, we are expected to read in every metric, right?
Wouldn't it be better if probably we only pulled in metrics that we have listed in our metrics map that we pass in with 'New' .. that way we can ignore some metrics that we aren't interested in, etc?
There was a problem hiding this comment.
The general problem here is that any metrics solution has a pattern of local-reporting->aggregation->remote-reporting, but rcrowley metrics and m3 both want to do the aggregation step. So I have to de-aggregate from rcrowley to allow m3 to aggregate.
Regarding the Each() call, this is just saying that I get a stab at handling all metrics that Sarama reports through rcrowley/go-metrics. I don't handle all of them. The unhandled ones are periodically (but minimally) reported in the unexported metrics section. I include that unexported reporting, since there may come a time where those metrics are useful for some kind of debugging. Right now, it's a bunch of broker-specific metrics, which I fear will exceed the cardinality limits that M3 imposes on us. See https://godoc.org/github.com/Shopify/sarama
For the handled metrics, I need to de-aggregate them in various ways, and then map them to the equivalent M3 stuff.
| } | ||
|
|
||
| // Run runs the exporter. It blocks until Stop() is called, so it should probably be run in a go-routine | ||
| func (r *GoMetricsExporter) Run() { |
There was a problem hiding this comment.
having the caller do a "go Run()" seems to unnecessarily expose internal behaviour, etc. I would (again) suggest including a "Start()" method that does a "go r.run()" internally. That complements the "Stop()" you have, too.
No description provided.