-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
statsd-emitter #2410
statsd-emitter #2410
Conversation
598802c
to
fb1767e
Compare
</dependency> | ||
</dependencies> | ||
|
||
</project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line
need more UTs like ser/deser. |
private final Map<String, StatsDEmitterConfig.MetricType> metricTypes; | ||
|
||
public StatsDEmitter(StatsDEmitterConfig config) { | ||
statsd = new NonBlockingStatsDClient(config.getPrefix(), config.getHostname(), config.getPort()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use this instead https://github.com/tim-group/java-statsd-client/blob/master/src/main/java/com/timgroup/statsd/NonBlockingStatsDClient.java#L89 like that you have a handler of exception.
@michaelschiff again my main concern is the fact that StatD will do the aggregation of a huge amount of metrics with a very very high cardinality probably onHeap. This can blow the druid process. I think you need a way to filter out the dimensions with high cardinality like query id. |
@b-slim this is statd's stated purpose. Also 0 aggregation is done in in the emitter, so I am not sure why you are concerned this will "blow away the druid process". As we discussed (at great length) in the Ganglia pull request, this type of filtering does not really solve its intended problem. My reasoning regarding the cardinality problem was as such:
|
d458bb5
to
31e1179
Compare
747eaa8
to
35c96e9
Compare
35c96e9
to
7c780dc
Compare
Hi all, Things that we discussed /changed internally at our company:
Somewhat off-topic for the scope of this pull request but related to our testing:
|
one more: it would be nice to have a counter metric for the alert-typed metrics. It would be possible to build this into the statsd-emitter or one could build it into Druid. |
@sascha-coenen I totally agree! Point 1 - We use codahale metrics everywhere in our company's applications, and it is a really nice library. In terms of druid, it would cover both the metric collection in the service, and then expose the Reporter interface which we might still end up implementing for each store (if they don't exist already). I could imagine this being a similar amount of code to a new Emitter. It would also kind of require rethinking how metrics are actually named (i.e. MetricName + combination of dimensions). You definitely don't want to end up storing a Metric in memory for each dimension combination.... Point 4 - I definitely felt this as I was defining the mapping. I did my best to make sure that every type is correct for the metric, but I may have made mistakes. While working on this, it occurred to me that this type information could be part of the Druid metric interface... Point 6 - Makes sense, that's more convenient. |
This was mentioned in the original pull (#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue #3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
This was mentioned in the original pull (apache/druid#2410) by @sascha-coenen, and the original author (@michaelschiff) agreed that it seemed reasonable This commit fixes issue apache/druid#3960
After spending some time trying to emit metrics directly from Druid to Ganglia, it became clear that Druid needs some kind of pre-aggregation step before loading metrics into such a store. This type of aggregation seemed beyond the scope of a Metric Emitter (which should really just emit metrics, not do fancy, configurable aggregations), and is exactly what StatsD is designed for.
I am still testing this emitter, but would like to open the PR now so that I can get some eyes from the community on what I am doing.