Consolidate query metrics and include result tag (#1075)

* Consolidate query metrics and include result tag Signed-off-by: Gary Brown <gary@brownuk.com> * Add changelog entry and remove 'Attempts' metric as no longer used Signed-off-by: Gary Brown <gary@brownuk.com> * Just name the main request count based on the operation being performed Signed-off-by: Gary Brown <gary@brownuk.com>
jaegertracing · Sep 28, 2018 · b2aa771 · b2aa771
1 parent f441f1d
commit b2aa771
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 39 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,17 @@
 Changes by Version
 ==================
 
+1.8.0 (unreleased)
+------------------
+
+#### Backend Changes
+
+##### Breaking Changes
+
+- Consolidate query metrics and include result tag ([#1075](https://github.com/jaegertracing/jaeger/pull/1075), [@objectiser](https://github.com/objectiser)
+- Make the metrics produced by jaeger query scoped to the query component, and generated for all span readers (not just ES) ([#1074](https://github.com/jaegertracing/jaeger/pull/1074), [@objectiser](https://github.com/objectiser)
+
+
 1.7.0 (2018-09-19)
 ------------------
 

diff --git a/storage/spanstore/metrics/decorator.go b/storage/spanstore/metrics/decorator.go
@@ -34,16 +34,14 @@ type ReadMetricsDecorator struct {
 }
 
 type queryMetrics struct {
-	Errors     metrics.Counter `metric:"errors"`
-	Attempts   metrics.Counter `metric:"attempts"`
-	Successes  metrics.Counter `metric:"successes"`
-	Responses  metrics.Timer   `metric:"responses"` //used as a histogram, not necessary for GetTrace
-	ErrLatency metrics.Timer   `metric:"errLatency"`
-	OKLatency  metrics.Timer   `metric:"okLatency"`
+	Errors     metrics.Counter
+	Successes  metrics.Counter
+	Responses  metrics.Timer //used as a histogram, not necessary for GetTrace
+	ErrLatency metrics.Timer
+	OKLatency  metrics.Timer
 }
 
 func (q *queryMetrics) emit(err error, latency time.Duration, responses int) {
-	q.Attempts.Inc(1)
 	if err != nil {
 		q.Errors.Inc(1)
 		q.ErrLatency.Record(latency)
@@ -66,9 +64,14 @@ func NewReadMetricsDecorator(spanReader spanstore.Reader, metricsFactory metrics
 }
 
 func buildQueryMetrics(namespace string, metricsFactory metrics.Factory) *queryMetrics {
-	qMetrics := &queryMetrics{}
 	scoped := metricsFactory.Namespace(namespace, nil)
-	metrics.Init(qMetrics, scoped, nil)
+	qMetrics := &queryMetrics{
+		Errors:     scoped.Counter("", map[string]string{"result": "err"}),
+		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"}),
+	}
 	return qMetrics
 }
 

diff --git a/storage/spanstore/metrics/decorator_test.go b/storage/spanstore/metrics/decorator_test.go
@@ -43,27 +43,23 @@ func TestSuccessfulUnderlyingCalls(t *testing.T) {
 	mrs.FindTraces(context.Background(), &spanstore.TraceQueryParameters{})
 	counters, gauges := mf.Snapshot()
 	expecteds := map[string]int64{
-		"get_operations.attempts":  1,
-		"get_operations.successes": 1,
-		"get_operations.errors":    0,
-		"get_trace.attempts":       1,
-		"get_trace.successes":      1,
-		"get_trace.errors":         0,
-		"find_traces.attempts":     1,
-		"find_traces.successes":    1,
-		"find_traces.errors":       0,
-		"get_services.attempts":    1,
-		"get_services.successes":   1,
-		"get_services.errors":      0,
+		"get_operations|result=ok":  1,
+		"get_operations|result=err": 0,
+		"get_trace|result=ok":       1,
+		"get_trace|result=err":      0,
+		"find_traces|result=ok":     1,
+		"find_traces|result=err":    0,
+		"get_services|result=ok":    1,
+		"get_services|result=err":   0,
 	}
 
 	existingKeys := []string{
-		"get_operations.okLatency.P50",
+		"get_operations.latency|result=ok.P50",
 		"get_trace.responses.P50",
-		"find_traces.okLatency.P50", // this is not exhaustive
+		"find_traces.latency|result=ok.P50", // this is not exhaustive
 	}
 	nonExistentKeys := []string{
-		"get_operations.errLatency.P50",
+		"get_operations.latency|result=err.P50",
 	}
 
 	checkExpectedExistingAndNonExistentCounters(t, counters, expecteds, gauges, existingKeys, nonExistentKeys)
@@ -100,28 +96,24 @@ func TestFailingUnderlyingCalls(t *testing.T) {
 	mrs.FindTraces(context.Background(), &spanstore.TraceQueryParameters{})
 	counters, gauges := mf.Snapshot()
 	expecteds := map[string]int64{
-		"get_operations.attempts":  1,
-		"get_operations.successes": 0,
-		"get_operations.errors":    1,
-		"get_trace.attempts":       1,
-		"get_trace.successes":      0,
-		"get_trace.errors":         1,
-		"find_traces.attempts":     1,
-		"find_traces.successes":    0,
-		"find_traces.errors":       1,
-		"get_services.attempts":    1,
-		"get_services.successes":   0,
-		"get_services.errors":      1,
+		"get_operations|result=ok":  0,
+		"get_operations|result=err": 1,
+		"get_trace|result=ok":       0,
+		"get_trace|result=err":      1,
+		"find_traces|result=ok":     0,
+		"find_traces|result=err":    1,
+		"get_services|result=ok":    0,
+		"get_services|result=err":   1,
 	}
 
 	existingKeys := []string{
-		"get_operations.errLatency.P50",
+		"get_operations.latency|result=err.P50",
 	}
 
 	nonExistentKeys := []string{
-		"get_operations.okLatency.P50",
+		"get_operations.latency|result=ok.P50",
 		"get_trace.responses.P50",
-		"query.okLatency.P50", // this is not exhaustive
+		"query.latency|result=ok.P50", // this is not exhaustive
 	}
 
 	checkExpectedExistingAndNonExistentCounters(t, counters, expecteds, gauges, existingKeys, nonExistentKeys)