Normalize metric names and tags #222

yurishkuro · 2017-11-12T03:33:54Z

When used with Prometheus backend current metrics cannot be initialized because they use the same names (e.g. "spans") with different dimensions of the tags (e.g. "group, state" vs. "group, sampled").

This change splits different sets of metrics by name, so that a single name uses the same dimensions of tags.

codecov · 2017-11-12T03:48:46Z

Codecov Report

Merging #222 into master will decrease coverage by 0.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
- Coverage   81.99%   81.74%   -0.25%     
==========================================
  Files          51       51              
  Lines        3226     3226              
==========================================
- Hits         2645     2637       -8     
- Misses        457      464       +7     
- Partials      124      125       +1

Impacted Files	Coverage Δ
metrics.go	`100% <100%> (ø)`	⬆️
tracer.go	`88.16% <100%> (-1.68%)`	⬇️
tracer_options.go	`73.52% <0%> (-5.89%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc11c1a...14c449e. Read the comment docs.

NeoCN · 2017-11-12T11:40:27Z

encounter with panic: duplicate metrics collector registration attempted. when use prometheus on client side, can not wait to see this pr and jaegertracing/jaeger-lib#29 being merged.

black-adder · 2017-11-12T19:53:01Z

README.md

-    stats := // create StatsReporter implementation
-    tracer := config.Tracing.New("your-service-name", stats)
+import (
+	"github.com/prometheus/client_golang/prometheus"


spacing is funky

black-adder · 2017-11-12T19:56:15Z

README.md

-HTTP Header `jaeger-baggage` on a request
+The OpenTracing spec allows for [baggage][baggage], which are key value pairs that are added
+to the span context and propagated throughout the trace. An external process can inject baggage
+by setting the special HTTP Header `jaeger-baggage` on a request:


we should have a TODO to update documentation so that users are aware of https://github.com/jaegertracing/jaeger-client-go/blob/master/header.go#L30

black-adder · 2017-11-12T20:58:58Z

metrics.go

@@ -33,16 +33,16 @@ type Metrics struct {
 	TracesJoinedNotSampled metrics.Counter `metric:"traces" tags:"state=joined,sampled=n"`

 	// Number of sampled spans started by this tracer
-	SpansStarted metrics.Counter `metric:"spans" tags:"group=lifecycle,state=started"`
+	SpansStarted metrics.Counter `metric:"spans-by-lifecycle" tags:"state=started"`


how bout "spans-lifecycle"

I'm open to suggestions how to make the naming more consistent across the board.

One other thing I would like to do is to use underscore instead of dash as a separator. Dash is not valid in Prometheus.

yurishkuro · 2017-11-14T19:07:57Z

@black-adder please take a look at the new naming https://github.com/jaegertracing/jaeger-client-go/pull/222/files#diff-c47669012d7664f9cbee69c67bf8bedfR36

Once we agree on it I will update the tests.

yurishkuro · 2017-11-15T00:51:04Z

metrics.go


 	// Number of times baggage restrictions failed to update.
-	BaggageRestrictionsUpdateFailure metrics.Counter `metric:"baggage-restrictions-update" tags:"result=err"`
+	BaggageRestrictionsUpdateFailure metrics.Counter `metric:"baggage_restrictions_updates" tags:"result=err"`
 }


@pavolloffay @objectiser penny for your thoughts.

+1 - I assume the names will be consistent across all clients?

As much as possible. I don't think we have tag-based APIs in Python and Node.js clients.

Another option would be to drop tags completely and just encode them in the name, e.g. traces_started_sampled. It makes it harder to aggregate if say you don't care about the "is_sampled" dimension.

Better to have tags - is there any reason they can't be added to python and node clients?

NeoCN · 2017-11-21T03:09:07Z

Waiting for this PR, any update?

yurishkuro · 2017-11-21T03:52:12Z

eta Nov 27

Signed-off-by: Yuri Shkuro <ys@uber.com>

black-adder · 2017-11-21T19:21:45Z

README.md

-    tracer := config.Tracing.New("your-service-name", stats)
+import (
+    "github.com/uber/jaeger-lib/metrics/prometheus"
+    "github.com/uber/jaeger-client-go/config"


omg imports

black-adder · 2017-11-21T19:23:14Z

metrics.go

@@ -33,63 +33,61 @@ type Metrics struct {
 	TracesJoinedNotSampled metrics.Counter `metric:"traces" tags:"state=joined,sampled=n"`

 	// Number of sampled spans started by this tracer
-	SpansStarted metrics.Counter `metric:"spans" tags:"group=lifecycle,state=started"`
+	SpansStartedSampled metrics.Counter `metric:"spans_started" tags:"sampled=y"`


I'd prefer sampled=true and sampled=false

black-adder · 2017-11-21T19:26:12Z

metrics_test.go

 			Tags:  map[string]string{"lib": "jaeger"},
 			Value: 11,
 		},
 	)
 }
+
+// TestNewPrometheusMetrics ensures that the metrics do not have conflicting dimensions and will work with Prometheus.
+func TestNewPrometheusMetrics(t *testing.T) {


remind me, will this pull in all of prometheus for service owners even if they don't use prometheus metrics?

I moved it to testImport: in glide. Another alternative is to move the test to a sub-package.

black-adder · 2017-11-21T21:13:20Z

metrics.go


 	// Number of times the Sampler succeeded to retrieve and update sampling strategy
-	SamplerUpdated metrics.Counter `metric:"sampler" tags:"state=updated"`
+	SamplerUpdated metrics.Counter `metric:"sampler_updated"`


"sampler_updates, result=ok" and "sampler_updates, result=err" to keep in line with the baggage stuff and we can update sampler_query_failures as you have.

Still leaves sampler_retrieved and baggage_truncated ending in a verb.

Also, if we want to convert everything to end with a noun, we may consider s/spans_started/started_spans/ (finished_spans, etc.)

Signed-off-by: Yuri Shkuro <ys@uber.com>

objectiser · 2017-11-27T16:46:09Z

metrics.go

@@ -33,40 +33,40 @@ type Metrics struct {
 	TracesJoinedNotSampled metrics.Counter `metric:"traces" tags:"state=joined,sampled=n"`

 	// Number of sampled spans started by this tracer
-	SpansStartedSampled metrics.Counter `metric:"spans_started" tags:"sampled=y"`
+	SpansStartedSampled metrics.Counter `metric:"started_spans" tags:"sampled=y"`


Curious why this change? and similarly for finished_spans

wanted to normalize all names to end with noun

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro requested a review from black-adder November 12, 2017 03:33

yurishkuro force-pushed the normalize-metrics branch from 588c755 to 6163718 Compare November 12, 2017 03:48

black-adder approved these changes Nov 12, 2017

View reviewed changes

yurishkuro force-pushed the normalize-metrics branch from b61406c to 95314c3 Compare November 14, 2017 18:21

yurishkuro force-pushed the normalize-metrics branch from 121d54c to 8330758 Compare November 14, 2017 21:56

yurishkuro commented Nov 15, 2017

View reviewed changes

mumoshu mentioned this pull request Nov 20, 2017

Support Prometheus metrics directly jaegertracing/jaeger-lib#29

Merged

Yuri Shkuro added 4 commits November 21, 2017 13:54

Make metrics compatible with Prometheus

8141901

Signed-off-by: Yuri Shkuro <ys@uber.com>

Define new naming

0cbfbea

Signed-off-by: Yuri Shkuro <ys@uber.com>

Bump jaeger-lib

5bc8d15

Signed-off-by: Yuri Shkuro <ys@uber.com>

Fix tests

abd1f54

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro force-pushed the normalize-metrics branch from 8330758 to abd1f54 Compare November 21, 2017 18:56

Fix example

fed1ced

Signed-off-by: Yuri Shkuro <ys@uber.com>

black-adder reviewed Nov 21, 2017

View reviewed changes

Yuri Shkuro added 2 commits November 21, 2017 19:52

Move Prometheus to test dependency

4f1e032

Signed-off-by: Yuri Shkuro <ys@uber.com>

Normalize metric names even more

b6743c7

Signed-off-by: Yuri Shkuro <ys@uber.com>

objectiser reviewed Nov 27, 2017

View reviewed changes

Yuri Shkuro added 2 commits November 27, 2017 14:51

Fix tests

9add458

Signed-off-by: Yuri Shkuro <ys@uber.com>

Move prometheus test to a subdirectory

14c449e

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro merged commit 0e91c43 into master Nov 27, 2017

yurishkuro deleted the normalize-metrics branch November 27, 2017 20:20

yurishkuro mentioned this pull request Nov 28, 2017

Enable some form of metrics from Jaeger backend processes jaegertracing/jaeger#273

Closed

NeoCN mentioned this pull request Nov 30, 2017

Metrics related docs not synced with current code #216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize metric names and tags #222

Normalize metric names and tags #222

yurishkuro commented Nov 12, 2017

codecov bot commented Nov 12, 2017 •

edited

Loading

NeoCN commented Nov 12, 2017

black-adder Nov 12, 2017

black-adder Nov 12, 2017

yurishkuro Nov 12, 2017

black-adder Nov 12, 2017

yurishkuro Nov 12, 2017

yurishkuro commented Nov 14, 2017

yurishkuro Nov 15, 2017

objectiser Nov 15, 2017

yurishkuro Nov 16, 2017

objectiser Nov 16, 2017

NeoCN commented Nov 21, 2017

yurishkuro commented Nov 21, 2017

black-adder Nov 21, 2017

black-adder Nov 21, 2017

black-adder Nov 21, 2017

yurishkuro Nov 22, 2017

black-adder Nov 21, 2017

yurishkuro Nov 22, 2017

objectiser Nov 27, 2017

yurishkuro Nov 27, 2017

Normalize metric names and tags #222

Normalize metric names and tags #222

Conversation

yurishkuro commented Nov 12, 2017

codecov bot commented Nov 12, 2017 • edited Loading

Codecov Report

NeoCN commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeoCN commented Nov 21, 2017

yurishkuro commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 12, 2017 •

edited

Loading