addthis / metrics-reporter-config

Apache License 2.0
74 stars 52 forks source link

add prometheus support #26

Closed snazy closed 8 years ago

snazy commented 8 years ago

This proposed patch integrates metrics exporting for prometheus. It eliminates the need for the prometheus graphite-exporter (or any other exporter) between the monitored application and prometheus. In order to do that, it opens its own HTTP listener, which can optionally encrypted using a self-signed certificate. (More SSL support could be added on top of that.)

The patch integrates both into yammer and codahale modules.

An example reporter/exporter config is included for Apache Cassandra and has been tested with versions 2.1.x and 3.x.

A big section of the configuration (as in the example config) is dedicated to map yammer/codahale metric names to prometheus names and labels.

yuesong commented 8 years ago

Thanks for your pull request. Prometheus support would make a great addition.

I got a maven build error because of protobuf toolchain is not setup locally. I haven't had much experience using protobuf in my projects, so it took me a bit to figure it out. I think the dependency on this extra setup outside of the project/pom is not ideal, especially given that it is only needed for one of the reporters, so I'd like to explore alternatives:

  1. Include the compiled java class directly in the project. It does require extra manual work for devs when making related changes in the future, but it reduces confusion and external dependencies for majority of the users.
  2. Include the protoc executable in the project and use the protocExecutable config of the protobuf-maven-plugin, but there's the os issue, not to mention the bad taste of checking in an executable in source control
  3. Something rather involved like this: http://vlkan.com/blog/post/2015/11/27/maven-protobuf/

To me # 1 seems to be the best option. Although it may go against the best practices of using protobuf in general, it's probably the best compromise for this project.

Yuesong

jhorwit2 commented 8 years ago

I have a few questions. Why are you rolling your own metrics transformation code when prometheus provides a decorator class for metrics3? The prometheus devs also provide a push library, so why not use that? Reusing that code seems like a better approach than rolling our own.

https://github.com/prometheus/client_java

Please see pushgateway & the dropwizard modules.

Perhaps prometheus support in this library should remain with metrics3.

jhorwit2 commented 8 years ago

If you would like to stick with the protobuf support it seems the prometheus devs provide a compiled java class for us here

jhorwit2 commented 8 years ago

@snazy any updates on this? This is actually something we will find very useful soon, so if you don't have the time I can take over in another PR.

snazy commented 8 years ago

@jhorwit2, I'm currently maxed out with other activities and unfortunately have very little time for this one. Happy to hand this over to you!

Well, the decorator approach only works for metrics3 but not the old metrics code, which is still used in the wild (e.g. Apache Cassandra 2.x).

Protobuf: I've a preference for the protobuf version as it is cheaper to handle than the text format.

Pushgateway: I assume your use case is about short running (batch) jobs?

discordianfish commented 8 years ago

Hi,

would be great to have this! In general it probably makes sense to use the prometheus client lib decorators but not the push library. Prometheus encourages to use push only for short running jobs. The right way is to have your service provide the prometheus metrics on a endpoint so the prometheus server can scrape it at whatever interval is configured.

jhorwit2 commented 8 years ago

@discordianfish That would require an api change to support passing the environment into the reporter config to add a servlet. If you want to expose the endpoint I would suggest you use the dropwizard decorator in combination with the metric servlet. Should only be a couple LOC to make it work.

jhorwit2 commented 8 years ago

To be exact, you need these two modules

            <dependency>
                <groupId>io.prometheus</groupId>
                <artifactId>simpleclient_servlet</artifactId>
                <version>0.0.16</version>
            </dependency>

            <dependency>
                <groupId>io.prometheus</groupId>
                <artifactId>simpleclient_dropwizard</artifactId>
                <version>0.0.16</version>
            </dependency>

and to add this to your application run.

CollectorRegistry registry = new CollectorRegistry();
registry.register(new DropwizardExports(state.getMetricRegistry()));
environment.admin()
    .addServlet("prometheus-metrics", new MetricsServlet(registry))
    .addMapping("/prometheus-metrics");
jhorwit2 commented 8 years ago

Actually after playing with that it's not great since it doesn't let you specify labels/values. I'll see about incorporating the servlet option into this.

jhorwit2 commented 8 years ago

Closing this in favor of https://github.com/addthis/metrics-reporter-config/pull/33.

@discordianfish I added the ability to do servlet type instead of push gateway; however, you will need to register the servlet yourself. I didn't want to pull in that dependency (dropwizard). You can look at the readme for an example.