From 8f1956a7a815255049f617a1eaa6b021c73e9204 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Sat, 29 Sep 2018 10:44:51 +0100 Subject: [PATCH 1/4] Specify metric name/tags via annotation Signed-off-by: Gary Brown --- storage/spanstore/metrics/decorator.go | 19 +++++------- storage/spanstore/metrics/decorator_test.go | 32 ++++++++++----------- 2 files changed, 23 insertions(+), 28 deletions(-) diff --git a/storage/spanstore/metrics/decorator.go b/storage/spanstore/metrics/decorator.go index c013ddda5b5..ff5ec93fa6e 100644 --- a/storage/spanstore/metrics/decorator.go +++ b/storage/spanstore/metrics/decorator.go @@ -34,11 +34,11 @@ type ReadMetricsDecorator struct { } type queryMetrics struct { - Errors metrics.Counter - Successes metrics.Counter - Responses metrics.Timer //used as a histogram, not necessary for GetTrace - ErrLatency metrics.Timer - OKLatency metrics.Timer + Errors metrics.Counter `metric:"calls" tags:"result=err"` + Successes metrics.Counter `metric:"calls" tags:"result=ok"` + Responses metrics.Timer `metric:"responses"` //used as a histogram, not necessary for GetTrace + ErrLatency metrics.Timer `metric:"latency" tags:"result=err"` + OKLatency metrics.Timer `metric:"latency" tags:"result=ok"` } func (q *queryMetrics) emit(err error, latency time.Duration, responses int) { @@ -64,14 +64,9 @@ func NewReadMetricsDecorator(spanReader spanstore.Reader, metricsFactory metrics } func buildQueryMetrics(namespace string, metricsFactory metrics.Factory) *queryMetrics { + qMetrics := &queryMetrics{} scoped := metricsFactory.Namespace(namespace, 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"}), - } + metrics.Init(qMetrics, scoped, nil) return qMetrics } diff --git a/storage/spanstore/metrics/decorator_test.go b/storage/spanstore/metrics/decorator_test.go index 0fe9f352fe4..22a9c24989b 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|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, + "get_operations.calls|result=ok": 1, + "get_operations.calls|result=err": 0, + "get_trace.calls|result=ok": 1, + "get_trace.calls|result=err": 0, + "find_traces.calls|result=ok": 1, + "find_traces.calls|result=err": 0, + "get_services.calls|result=ok": 1, + "get_services.calls|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|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, + "get_operations.calls|result=ok": 0, + "get_operations.calls|result=err": 1, + "get_trace.calls|result=ok": 0, + "get_trace.calls|result=err": 1, + "find_traces.calls|result=ok": 0, + "find_traces.calls|result=err": 1, + "get_services.calls|result=ok": 0, + "get_services.calls|result=err": 1, } existingKeys := []string{ From 7839d7d4b278a50cca84fa89c7112c22682790a9 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Sat, 29 Sep 2018 13:26:33 +0100 Subject: [PATCH 2/4] Update changelog Signed-off-by: Gary Brown --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2755cf0f91..30392226d0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,8 @@ Changes by Version ##### 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) +- Consolidate query metrics and include result tag ([#1075](https://github.com/jaegertracing/jaeger/pull/1075) and [#1096](https://github.com/jaegertracing/jaeger/pull/1096), [@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) From 5ba1d312d1ffc28bd23bf7cd28a8958ab830e167 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Sun, 30 Sep 2018 11:38:59 +0100 Subject: [PATCH 3/4] Revert back to 'requests' and make the operation name a tag Signed-off-by: Gary Brown --- storage/spanstore/metrics/decorator.go | 8 ++-- storage/spanstore/metrics/decorator_test.go | 48 ++++++++++----------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/storage/spanstore/metrics/decorator.go b/storage/spanstore/metrics/decorator.go index ff5ec93fa6e..dcf256d18bb 100644 --- a/storage/spanstore/metrics/decorator.go +++ b/storage/spanstore/metrics/decorator.go @@ -34,8 +34,8 @@ type ReadMetricsDecorator struct { } type queryMetrics struct { - Errors metrics.Counter `metric:"calls" tags:"result=err"` - Successes metrics.Counter `metric:"calls" tags:"result=ok"` + Errors metrics.Counter `metric:"requests" tags:"result=err"` + Successes metrics.Counter `metric:"requests" tags:"result=ok"` Responses metrics.Timer `metric:"responses"` //used as a histogram, not necessary for GetTrace ErrLatency metrics.Timer `metric:"latency" tags:"result=err"` OKLatency metrics.Timer `metric:"latency" tags:"result=ok"` @@ -63,9 +63,9 @@ func NewReadMetricsDecorator(spanReader spanstore.Reader, metricsFactory metrics } } -func buildQueryMetrics(namespace string, metricsFactory metrics.Factory) *queryMetrics { +func buildQueryMetrics(operation string, metricsFactory metrics.Factory) *queryMetrics { qMetrics := &queryMetrics{} - scoped := metricsFactory.Namespace(namespace, nil) + scoped := metricsFactory.Namespace("", map[string]string{"operation": operation}) metrics.Init(qMetrics, scoped, nil) return qMetrics } diff --git a/storage/spanstore/metrics/decorator_test.go b/storage/spanstore/metrics/decorator_test.go index 22a9c24989b..93121d3949e 100644 --- a/storage/spanstore/metrics/decorator_test.go +++ b/storage/spanstore/metrics/decorator_test.go @@ -43,23 +43,23 @@ func TestSuccessfulUnderlyingCalls(t *testing.T) { mrs.FindTraces(context.Background(), &spanstore.TraceQueryParameters{}) counters, gauges := mf.Snapshot() expecteds := map[string]int64{ - "get_operations.calls|result=ok": 1, - "get_operations.calls|result=err": 0, - "get_trace.calls|result=ok": 1, - "get_trace.calls|result=err": 0, - "find_traces.calls|result=ok": 1, - "find_traces.calls|result=err": 0, - "get_services.calls|result=ok": 1, - "get_services.calls|result=err": 0, + "requests|operation=get_operations|result=ok": 1, + "requests|operation=get_operations|result=err": 0, + "requests|operation=get_trace|result=ok": 1, + "requests|operation=get_trace|result=err": 0, + "requests|operation=find_traces|result=ok": 1, + "requests|operation=find_traces|result=err": 0, + "requests|operation=get_services|result=ok": 1, + "requests|operation=get_services|result=err": 0, } existingKeys := []string{ - "get_operations.latency|result=ok.P50", - "get_trace.responses.P50", - "find_traces.latency|result=ok.P50", // this is not exhaustive + "latency|operation=get_operations|result=ok.P50", + "responses|operation=get_trace.P50", + "latency|operation=find_traces|result=ok.P50", // this is not exhaustive } nonExistentKeys := []string{ - "get_operations.latency|result=err.P50", + "latency|operation=get_operations|result=err.P50", } checkExpectedExistingAndNonExistentCounters(t, counters, expecteds, gauges, existingKeys, nonExistentKeys) @@ -96,24 +96,24 @@ func TestFailingUnderlyingCalls(t *testing.T) { mrs.FindTraces(context.Background(), &spanstore.TraceQueryParameters{}) counters, gauges := mf.Snapshot() expecteds := map[string]int64{ - "get_operations.calls|result=ok": 0, - "get_operations.calls|result=err": 1, - "get_trace.calls|result=ok": 0, - "get_trace.calls|result=err": 1, - "find_traces.calls|result=ok": 0, - "find_traces.calls|result=err": 1, - "get_services.calls|result=ok": 0, - "get_services.calls|result=err": 1, + "requests|operation=get_operations|result=ok": 0, + "requests|operation=get_operations|result=err": 1, + "requests|operation=get_trace|result=ok": 0, + "requests|operation=get_trace|result=err": 1, + "requests|operation=find_traces|result=ok": 0, + "requests|operation=find_traces|result=err": 1, + "requests|operation=get_services|result=ok": 0, + "requests|operation=get_services|result=err": 1, } existingKeys := []string{ - "get_operations.latency|result=err.P50", + "latency|operation=get_operations|result=err.P50", } nonExistentKeys := []string{ - "get_operations.latency|result=ok.P50", - "get_trace.responses.P50", - "query.latency|result=ok.P50", // this is not exhaustive + "latency|operation=get_operations|result=ok.P50", + "responses|operation=get_trace.P50", + "latency|operation=query|result=ok.P50", // this is not exhaustive } checkExpectedExistingAndNonExistentCounters(t, counters, expecteds, gauges, existingKeys, nonExistentKeys) From 1796547c6ddc4e0044b66d4d0a724936cc73038c Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Mon, 1 Oct 2018 09:51:12 +0100 Subject: [PATCH 4/4] Add more details to the changelog Signed-off-by: Gary Brown --- CHANGELOG.md | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30392226d0b..e6a6d8d7fa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,28 @@ Changes by Version ##### Breaking Changes -- Consolidate query metrics and include result tag ([#1075](https://github.com/jaegertracing/jaeger/pull/1075) and [#1096](https://github.com/jaegertracing/jaeger/pull/1096), [@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)) +- Various changes around metrics produced by jaeger-query: Names scoped to the query component, generated for all span readers (not just ES), consolidate query metrics and include result tag ([#1074](https://github.com/jaegertracing/jaeger/pull/1074), [#1075](https://github.com/jaegertracing/jaeger/pull/1075) and [#1096](https://github.com/jaegertracing/jaeger/pull/1096), [@objectiser](https://github.com/objectiser)) + +For example, sample of metrics produced for `find_traces` operation before: + +``` +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 +``` + +And now: + +``` +jaeger_query_latency_bucket{operation="find_traces",result="err",le="0.005"} 0 +jaeger_query_latency_bucket{operation="find_traces",result="ok",le="0.005"} 2 +jaeger_query_requests{operation="find_traces",result="err"} 0 +jaeger_query_requests{operation="find_traces",result="ok"} 2 +jaeger_query_responses_bucket{operation="find_traces",le="0.005"} 2 +``` 1.7.0 (2018-09-19)