From aab7dca90ddc8ec91d4c4d912c09bee8c6151b77 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Thu, 20 Sep 2018 13:59:21 +0100 Subject: [PATCH 1/3] Consolidate query metrics and include result tag Signed-off-by: Gary Brown --- storage/spanstore/metrics/decorator.go | 22 +++++---- storage/spanstore/metrics/decorator_test.go | 52 +++++++++------------ 2 files changed, 35 insertions(+), 39 deletions(-) diff --git a/storage/spanstore/metrics/decorator.go b/storage/spanstore/metrics/decorator.go index e277253c206..3800e56c554 100644 --- a/storage/spanstore/metrics/decorator.go +++ b/storage/spanstore/metrics/decorator.go @@ -34,16 +34,15 @@ 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 + Attempts 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 +65,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("requests", map[string]string{"result": "err"}), + Successes: scoped.Counter("requests", 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 index 9be83f7a696..b5db7b19e15 100644 --- 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.requests|result=ok": 1, + "get_operations.requests|result=err": 0, + "get_trace.requests|result=ok": 1, + "get_trace.requests|result=err": 0, + "find_traces.requests|result=ok": 1, + "find_traces.requests|result=err": 0, + "get_services.requests|result=ok": 1, + "get_services.requests|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.requests|result=ok": 0, + "get_operations.requests|result=err": 1, + "get_trace.requests|result=ok": 0, + "get_trace.requests|result=err": 1, + "find_traces.requests|result=ok": 0, + "find_traces.requests|result=err": 1, + "get_services.requests|result=ok": 0, + "get_services.requests|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) From 189c571c414fcae6839aef29fa3ba14a2afd1c0e Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Fri, 28 Sep 2018 14:21:08 +0100 Subject: [PATCH 2/3] Add changelog entry and remove 'Attempts' metric as no longer used Signed-off-by: Gary Brown --- CHANGELOG.md | 11 +++++++++++ storage/spanstore/metrics/decorator.go | 1 - 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9d35217891..c2755cf0f91 100644 --- 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 index 3800e56c554..ec3ceea8ec0 100644 --- a/storage/spanstore/metrics/decorator.go +++ b/storage/spanstore/metrics/decorator.go @@ -35,7 +35,6 @@ type ReadMetricsDecorator struct { type queryMetrics struct { Errors metrics.Counter - Attempts metrics.Counter Successes metrics.Counter Responses metrics.Timer //used as a histogram, not necessary for GetTrace ErrLatency metrics.Timer From 03b899acaad512d7b4a238d8e57f4fa967d68e26 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Fri, 28 Sep 2018 14:26:32 +0100 Subject: [PATCH 3/3] Just name the main request count based on the operation being performed Signed-off-by: Gary Brown --- storage/spanstore/metrics/decorator.go | 4 +-- storage/spanstore/metrics/decorator_test.go | 32 ++++++++++----------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/storage/spanstore/metrics/decorator.go b/storage/spanstore/metrics/decorator.go index ec3ceea8ec0..c013ddda5b5 100644 --- a/storage/spanstore/metrics/decorator.go +++ b/storage/spanstore/metrics/decorator.go @@ -66,8 +66,8 @@ func NewReadMetricsDecorator(spanReader spanstore.Reader, metricsFactory metrics func buildQueryMetrics(namespace string, metricsFactory metrics.Factory) *queryMetrics { scoped := metricsFactory.Namespace(namespace, nil) qMetrics := &queryMetrics{ - Errors: scoped.Counter("requests", map[string]string{"result": "err"}), - Successes: scoped.Counter("requests", map[string]string{"result": "ok"}), + 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"}), diff --git a/storage/spanstore/metrics/decorator_test.go b/storage/spanstore/metrics/decorator_test.go index b5db7b19e15..0fe9f352fe4 100644 --- a/storage/spanstore/metrics/decorator_test.go +++ b/storage/spanstore/metrics/decorator_test.go @@ -43,14 +43,14 @@ func TestSuccessfulUnderlyingCalls(t *testing.T) { mrs.FindTraces(context.Background(), &spanstore.TraceQueryParameters{}) counters, gauges := mf.Snapshot() expecteds := map[string]int64{ - "get_operations.requests|result=ok": 1, - "get_operations.requests|result=err": 0, - "get_trace.requests|result=ok": 1, - "get_trace.requests|result=err": 0, - "find_traces.requests|result=ok": 1, - "find_traces.requests|result=err": 0, - "get_services.requests|result=ok": 1, - "get_services.requests|result=err": 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{ @@ -96,14 +96,14 @@ func TestFailingUnderlyingCalls(t *testing.T) { mrs.FindTraces(context.Background(), &spanstore.TraceQueryParameters{}) counters, gauges := mf.Snapshot() expecteds := map[string]int64{ - "get_operations.requests|result=ok": 0, - "get_operations.requests|result=err": 1, - "get_trace.requests|result=ok": 0, - "get_trace.requests|result=err": 1, - "find_traces.requests|result=ok": 0, - "find_traces.requests|result=err": 1, - "get_services.requests|result=ok": 0, - "get_services.requests|result=err": 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{