Use upstream Prometheus client and Spring integration #1609

abesto · 2017-06-10T11:48:32Z

This change replaces our custom logic for exposing metrics with the implementation in the upstream Prometheus client, as noted in #1144 (comment). The notable improvement (other than less code) is the addition of histogram data of response times, using a servlet filter (the data looks something like this: https://gist.github.com/abesto/46aeb18ab45e5126ccebc515eb3fc99f)

Metrics comparison: before / after

There were two issues that blocked us from merging this for a while:

We're hit by PrometheusMvcEndpoint.java NPE for missing name parameter prometheus/client_java#252 (pops up only when Prometheus tries to query the data, you can still check out /prometheus from a browser just fine)
The upstream integration with Spring Boot Actuator metrics doesn't use the counter. and gauge. prefixes to figure out the right metric type, always defaults to gauge. Spring Boot counter metrics are now counters in Prometheus prometheus/client_java#254 aims to fix this.

abesto · 2017-06-11T05:38:40Z

The Spring counter metrics don't become, and shouldn't become Prometheus counter metrics, since in Spring Boot, counters can decrease. Current practice is to just make metrics like this gauges in Prometheus (prometheus/client_java#254 (comment)).

We should also see more JVM metrics exposed, in a way more Prometheus friendly way, letting up improve the dashboard further.

codefromthecrypt · 2017-06-12T06:45:18Z

zipkin-autoconfigure/metrics-prometheus/pom.xml

-            <scope>test</scope>
+          <groupId>io.prometheus</groupId>
+          <artifactId>simpleclient</artifactId>
+          <version>0.0.23</version>


seems ok to have an unstable dep here because the server-side dependency is optional (which makes it a part of the all-jar, but not a strict dep for customizers)

Sorry, I don't speak fluent Maven. By unstable dep, do you mean we shouldn't pin the version of io.prometheus:simpleclient? Or that I shouldn't remove org.springframework:spring-test?

unstable I meant the patch-only version number :)

nit: maybe use a property to coordinate these? ex.

in the properties section <simpleclient.version>0.0.23</simpleclient.version> then in places like here: ${simpleclient.version}

Ah, I see.

Yep, will set up a property for the version number.

codefromthecrypt · 2017-06-12T06:47:31Z

might be worth making some sample traffic against this to ensure that collector metrics show up (they are probably the most important metric). ex run one of the example projects like https://github.com/openzipkin/pyramid_zipkin-example

abesto · 2017-06-12T06:49:53Z

The upstream client exposes all the Spring Boot Actuator metrics, in almost exactly the same way as the previous Zipkin implementation. Still, due diligence is due, I'll check.

abesto · 2017-06-12T09:19:32Z

Metrics after sending a few spans using pyramid_zipkin-example: https://gist.github.com/abesto/93bd70bf907d16d3b7ef4e408408f69f

Methinks we're fine:

# HELP counter_zipkin_collector_messages_http counter_zipkin_collector_messages_http
# TYPE counter_zipkin_collector_messages_http gauge
counter_zipkin_collector_messages_http 13.0

they are probably the most important metric

Oh! Then I guess I should add them to the Grafana dashboard.

codefromthecrypt · 2017-06-12T09:21:54Z

they are probably the most important metric Oh! Then I guess I should add them to the Grafana dashboard!

yes I had a nag in the back of my head to nag you on the other issue. By naming convention substitute http for kafka (sqs scribe) etc as not everyone sends spans via http transport. messages, spans and bytes are interesting metrics, especially if coordinated client-side. drop metrics are something that one would alert on at some point.

abesto · 2017-06-12T09:40:39Z

Important note, just realized: this change does actually break the currently used metric names. The Zipkin implementation trims the counter_ and gauge_ prefixes (and sets the Prometheus metric type accordingly), while the upstream one does not trim those prefixes, and makes everything a gauge in Prometheus (see #1609 (comment)).

Assumption: not a lot of people are currently using these metrics. Proposal: let's drop as much custom logic as we can, and break the metric names now, before publishing the dashboard using the metric names.

codefromthecrypt · 2017-06-12T09:56:59Z

Assumption: not a lot of people are currently using these metrics. Proposal: let's drop as much custom logic as we can, and break the metric names now, before publishing the dashboard using the metric names.

I'd look at changelog for the directory and ping any implicated. After that go for it

…

On Mon, Jun 12, 2017 at 5:40 PM, Zoltán Nagy ***@***.***> wrote: Important note, just realized: this change does actually break the currently used metric names. The Zipkin implementation trims the counter_ and gauge_ prefixes (and sets the Prometheus metric type accordingly), while the upstream one does *not* trim those prefixes, and makes everything a gauge in Prometheus (see #1609 (comment) <#1609 (comment)>). Assumption: not a lot of people are currently using these metrics. Proposal: let's drop as much custom logic as we can, and break the metric names now, before publishing the dashboard using the metric names. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1609 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61xuKt-aXNt6rHIwJ_eeNonZcx50Tks5sDQeZgaJpZM4N2F3H> .

abesto · 2017-06-12T10:07:34Z

@klette Having implemented the original Zipkin Prometheus exporter, what are your thoughts on this PR? Especially #1609 (comment)?

(That exhausts the list of people who contributed to the Prometheus exporter 😄 )

codefromthecrypt · 2017-06-28T20:03:53Z

zipkin-autoconfigure/metrics-prometheus/pom.xml

@@ -27,6 +27,7 @@

    <properties>
        <main.basedir>${project.basedir}/../..</main.basedir>
+        <prometheus_simpleclient.version>0.0.23</prometheus_simpleclient.version>


the fix to the aforementioned bug is in master now, so guessing 0.0.24 will sort it out

0.0.26 is out, so probably time to see if we're sorted?

😨 Sad response time is sad. Yep, I'll check it out today.

Confirmed, Prometheus can successfully scrape data with 0.0.26. Pushed a commit; I'll start updating the Grafana dashboard for the new metric names, upload once this is merged, then we can hopefully also merge openzipkin-attic/docker-zipkin#135

codefromthecrypt · 2017-08-17T01:48:42Z

@klette nagtime!

codefromthecrypt

Will merge after testing against dashboard

For prometheus/client_java#252

codefromthecrypt · 2017-10-10T08:31:41Z

gracias!

abesto mentioned this pull request Jun 11, 2017

Fix #121: Add example prometheus setup openzipkin-attic/docker-zipkin#135

Merged

codefromthecrypt reviewed Jun 12, 2017

View reviewed changes

codefromthecrypt reviewed Jun 28, 2017

View reviewed changes

abesto added a commit to openzipkin-attic/docker-zipkin that referenced this pull request Aug 16, 2017

[prometheus] Update rewrite rules for openzipkin/zipkin#1609

63bfa19

codefromthecrypt approved these changes Oct 10, 2017

View reviewed changes

abesto and others added 5 commits October 10, 2017 14:30

Use upstream Prometheus client and Spring integration

ef99830

[prometheus] Update README

f4cb428

Extract prometheus client lib version into property

8026367

Bump prometheus_simpleclient

fbd7299

For prometheus/client_java#252

polish

0104dde

codefromthecrypt force-pushed the prometheus-use-upstream branch from 1f91ddc to 0104dde Compare October 10, 2017 07:06

codefromthecrypt pushed a commit to openzipkin-attic/docker-zipkin that referenced this pull request Oct 10, 2017

[prometheus] Update rewrite rules for openzipkin/zipkin#1609

1caa146

codefromthecrypt merged commit ed2a05d into master Oct 10, 2017

codefromthecrypt deleted the prometheus-use-upstream branch October 10, 2017 08:31

abesto added a commit to abesto/zipkin that referenced this pull request Sep 10, 2019

Use upstream Prometheus client and Spring integration (openzipkin#1609)

a0ce613

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use upstream Prometheus client and Spring integration #1609

Use upstream Prometheus client and Spring integration #1609

abesto commented Jun 10, 2017 •

edited

Loading

abesto commented Jun 11, 2017 •

edited

Loading

codefromthecrypt Jun 12, 2017

abesto Jun 12, 2017

codefromthecrypt Jun 12, 2017

abesto Jun 12, 2017

codefromthecrypt commented Jun 12, 2017

abesto commented Jun 12, 2017

abesto commented Jun 12, 2017 •

edited

Loading

codefromthecrypt commented Jun 12, 2017 via email

abesto commented Jun 12, 2017

codefromthecrypt commented Jun 12, 2017 via email

abesto commented Jun 12, 2017 •

edited

Loading

codefromthecrypt Jun 28, 2017

codefromthecrypt Aug 6, 2017

abesto Aug 16, 2017

abesto Aug 16, 2017 •

edited

Loading

codefromthecrypt commented Aug 17, 2017

codefromthecrypt left a comment

codefromthecrypt commented Oct 10, 2017

Use upstream Prometheus client and Spring integration #1609

Use upstream Prometheus client and Spring integration #1609

Conversation

abesto commented Jun 10, 2017 • edited Loading

abesto commented Jun 11, 2017 • edited Loading

codefromthecrypt Jun 12, 2017

Choose a reason for hiding this comment

abesto Jun 12, 2017

Choose a reason for hiding this comment

codefromthecrypt Jun 12, 2017

Choose a reason for hiding this comment

abesto Jun 12, 2017

Choose a reason for hiding this comment

codefromthecrypt commented Jun 12, 2017

abesto commented Jun 12, 2017

abesto commented Jun 12, 2017 • edited Loading

codefromthecrypt commented Jun 12, 2017 via email

abesto commented Jun 12, 2017

codefromthecrypt commented Jun 12, 2017 via email

abesto commented Jun 12, 2017 • edited Loading

codefromthecrypt Jun 28, 2017

Choose a reason for hiding this comment

codefromthecrypt Aug 6, 2017

Choose a reason for hiding this comment

abesto Aug 16, 2017

Choose a reason for hiding this comment

abesto Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

codefromthecrypt commented Aug 17, 2017

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Oct 10, 2017

abesto commented Jun 10, 2017 •

edited

Loading

abesto commented Jun 11, 2017 •

edited

Loading

abesto commented Jun 12, 2017 •

edited

Loading

abesto commented Jun 12, 2017 •

edited

Loading

abesto Aug 16, 2017 •

edited

Loading