Consolidate query metrics and include result tag #1075

objectiser · 2018-09-20T13:29:29Z

Signed-off-by: Gary Brown gary@brownuk.com

Which problem is this PR solving?

Currently jaeger-query produces a set of metrics for the following operations: find traces, get operations, get services and get trace.

The set of metrics are (only showing one bucket for histograms):

jaeger_find_traces_attempts 1
jaeger_find_traces_errLatency_bucket{le="0.005"} 0
jaeger_find_traces_errors 0
jaeger_find_traces_okLatency_bucket{le="0.005"} 0
jaeger_find_traces_responses_bucket{le="0.005"} 1
jaeger_find_traces_successes 1

Errors and successes for latency and counters are separated out (including a further counter representing the total counts).

This PR consolidates the metrics to use common name with a result tag representing ok or err states.

Short description of the changes

Have a single counter find_traces_requests with tag result (ok, err) to replace the attempts/errors/successes counters. So total (successes) is the count(find_traces_requests) and errors/successes can be determined by querying based on the tag.

Also combined the two latency based metrics and added a tag for result (ok/err).

jaeger_query_find_traces_latency_bucket{result="err",le="0.005"} 0
jaeger_query_find_traces_latency_bucket{result="ok",le="0.005"} 0
jaeger_query_find_traces_requests{result="err"} 0
jaeger_query_find_traces_requests{result="ok"} 0
jaeger_query_find_traces_responses_bucket{le="0.005"} 0

NOTE: Have not included the sum and count values for the histograms.

objectiser · 2018-09-20T13:33:35Z

storage/spanstore/metrics/decorator.go

 	scoped := metricsFactory.Namespace(namespace, nil)
-	metrics.Init(qMetrics, scoped, nil)
+	qMetrics := &queryMetrics{
+		Errors:     scoped.Counter("requests", map[string]string{"result": "err"}),


Is there a better name than requests and/or responses?
requests = number of query operations performed
responses = number of items returned per request

responses = number of items returned per request

what items? traces, spans?

It depends on which operation: find traces, get operations, get services and get trace.
So jaeger_find_traces_responses would be tracking the number of traces returned.

On reflection, requests and responses is probably ok - the requests is a counter, so should be obvious relates to number of requests. responses is a histogram to represent the number of items in each response.

Seems this discussion has finished already, but how about "operations" and "results"?

Even though "requests/responses" is probably OK as well, I would avoid it as it's a bit of a loaded term.

Have removed the additional part of the name for those counters, so now it is just:

jaeger_query_find_traces{result="err"} 0 jaeger_query_find_traces{result="ok"} 1 jaeger_query_find_traces_latency_bucket{result="err",le="0.005"} 0 jaeger_query_find_traces_latency_bucket{result="ok",le="0.005"} 1 jaeger_query_find_traces_responses_bucket{le="0.005"} 1

codecov · 2018-09-20T13:44:03Z

Codecov Report

Merging #1075 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1075   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         140     140           
  Lines        6622    6625    +3     
======================================
+ Hits         6622    6625    +3

Impacted Files	Coverage Δ
storage/spanstore/metrics/decorator.go	`100% <100%> (ø)`	⬆️

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 f441f1d...03b899a. Read the comment docs.

objectiser · 2018-09-24T16:40:19Z

@jaegertracing/jaeger-maintainers Updated description with the new equivalent metrics. Any thoughts on the new names/tags?

Signed-off-by: Gary Brown <gary@brownuk.com>

jpkrohling

LGTM, but could you also add an entry to the changelog, mentioning that this is a breaking change? People relying on an existing metric name will get affected by this change.

jpkrohling · 2018-09-28T11:34:34Z

storage/spanstore/metrics/decorator.go

 }

 func (q *queryMetrics) emit(err error, latency time.Duration, responses int) {
-	q.Attempts.Inc(1)


Is Attempts being used elsewhere?

jpkrohling · 2018-09-28T11:37:28Z

storage/spanstore/metrics/decorator.go

 	scoped := metricsFactory.Namespace(namespace, nil)
-	metrics.Init(qMetrics, scoped, nil)
+	qMetrics := &queryMetrics{
+		Errors:     scoped.Counter("requests", map[string]string{"result": "err"}),


Seems this discussion has finished already, but how about "operations" and "results"?

Even though "requests/responses" is probably OK as well, I would avoid it as it's a bit of a loaded term.

Signed-off-by: Gary Brown <gary@brownuk.com>

jpkrohling

LGTM

yurishkuro · 2018-09-28T17:13:33Z

storage/spanstore/metrics/decorator.go

+		Successes:  scoped.Counter("", map[string]string{"result": "ok"}),
+		Responses:  scoped.Timer("responses", nil),
+		ErrLatency: scoped.Timer("latency", map[string]string{"result": "err"}),
+		OKLatency:  scoped.Timer("latency", map[string]string{"result": "ok"}),


why not use the annotation-based initialization as before? It keeps declaration of the struct and metrics name in a single place.

Fixed in #1096

objectiser requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners September 20, 2018 13:29

ghost assigned objectiser Sep 20, 2018

ghost added the review label Sep 20, 2018

objectiser commented Sep 20, 2018

View reviewed changes

objectiser force-pushed the refactormetrics branch 2 times, most recently from 7663db2 to eda163f Compare September 24, 2018 16:15

Consolidate query metrics and include result tag

aab7dca

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the refactormetrics branch from eda163f to aab7dca Compare September 28, 2018 11:02

jpkrohling approved these changes Sep 28, 2018

View reviewed changes

objectiser added 2 commits September 28, 2018 14:21

Add changelog entry and remove 'Attempts' metric as no longer used

189c571

Signed-off-by: Gary Brown <gary@brownuk.com>

Just name the main request count based on the operation being performed

03b899a

Signed-off-by: Gary Brown <gary@brownuk.com>

jpkrohling approved these changes Sep 28, 2018

View reviewed changes

jpkrohling merged commit b2aa771 into jaegertracing:master Sep 28, 2018

ghost removed the review label Sep 28, 2018

yurishkuro reviewed Sep 28, 2018

View reviewed changes

objectiser mentioned this pull request Sep 29, 2018

Specify metric name/tags via annotation #1096

Merged

objectiser deleted the refactormetrics branch January 15, 2019 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate query metrics and include result tag #1075

Consolidate query metrics and include result tag #1075

objectiser commented Sep 20, 2018 •

edited

Loading

objectiser Sep 20, 2018

pavolloffay Sep 20, 2018

objectiser Sep 20, 2018

objectiser Sep 24, 2018

jpkrohling Sep 28, 2018

objectiser Sep 28, 2018

codecov bot commented Sep 20, 2018 •

edited

Loading

objectiser commented Sep 24, 2018

jpkrohling left a comment

jpkrohling Sep 28, 2018

jpkrohling Sep 28, 2018

jpkrohling left a comment

yurishkuro Sep 28, 2018

objectiser Sep 29, 2018

Consolidate query metrics and include result tag #1075

Consolidate query metrics and include result tag #1075

Conversation

objectiser commented Sep 20, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

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 Sep 20, 2018 • edited Loading

Codecov Report

objectiser commented Sep 24, 2018

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 20, 2018 •

edited

Loading