Prometheus Exemplars Support #448

EvgeniaMartynova-thebeat · 2021-12-21T14:57:14Z

Which problem is this PR solving?

Enabling Prometheus Exemplars. This PR is a copy of the POC made by @ioannissavvaidis.

Short description of the changes

Added support for Prometheus Exemplars
Updated Observarbility.md documentation
Enabled Prometheus Exemplars for request latency and counter metric in
- ../client/http/http.go
- ../client/amqp/v2/amqp.go
- ../client/es/elasticsearch.go
- ../client/grpc/grpc.go
- ../client/redis/redis.go
- ../client/sns/v2/publisher.go
- ../client/sql/publisher.go
- ../client/sns/v2/publisher.go
- ../component/amqp
- ../component/grpc
- ../component/kafka
- ..component/http/middleware
- ../component/sqs
- ../client/kafka/v2
Fixed a way how we record "success" metric. Mark metric entry as "success" if err == nil before we marked it as success if err != nil

mantzas

This PR handles only HTTP but we need to handle all possible cases here:

gRPC
Kafka
AMQP
etc
So this PR should include all of them

client/http/http.go

mantzas · 2021-12-21T15:18:27Z

client/http/http.go

+
+	traceID, ok := wct.ExtractTraceID(req.Context())
+	if ok {
+		durationHistogram.(prometheus.ExemplarObserver).ObserveWithExemplar(


There is a way to type assert safely which is:
v, ok := x.(T) // ok will be either true or false

…ting TraceID

tpaschalis · 2021-12-22T10:21:27Z

Hello @EvgeniaMartynova-thebeat and thanks for opening your first PR for Patron! 🎉 We really appreciate you taking the time to improve our shared tooling!

I have no idea what Prometheus Exemplars are all about (admittedly, I've got some reading to do 😄) but I wanted to clarify a couple of points.

Is this change backwards-compatible, or do we have to add a feature flag, an option, or something so that it can be enabled optionally?
Is this something that is specific for our Grafana-based observability stack or something that works generally?

siavashs · 2021-12-22T11:53:57Z

Hi @tpaschalis Exemplars are part of OpenMetrics Spec. They are an optional feature and as specified in the spec, the ingestors may ignore them so they are backwards compatible.
The observability tools (Prometheus, Thanos, Grafana) optionally support Exemplars https://grafana.com/docs/grafana/latest/basics/exemplars/

…pc, sql, redis, etc; put success label as err == nil for all of the clients

EvgeniaMartynova-thebeat · 2021-12-24T16:04:00Z

Hello @EvgeniaMartynova-thebeat and thanks for opening your first PR for Patron! 🎉 We really appreciate you taking the time to improve our shared tooling!

I have no idea what Prometheus Exemplars are all about (admittedly, I've got some reading to do 😄) but I wanted to clarify a couple of points.

Is this change backwards-compatible, or do we have to add a feature flag, an option, or something so that it can be enabled optionally?

Is this something that is specific for our Grafana-based observability stack or something that works generally?

Thank you for your quick review, @tpaschalis
Just an addition to what @siavashs said: the changes are backward-compatible when by enabling exemplars a record will be added after # for a metric entry like:

safety_perception_dynamodb_latency_seconds_bucket{method="GetPassengerProperties",le="0.005"} 2 # {traceID="4314cf66020d9262"} 2.4e-07 1.6367215682210166e+09

It is now part of Open Metrics format: https://grafana.com/blog/2021/03/31/intro-to-exemplars-which-enable-grafana-tempos-distributed-tracing-at-massive-scale/

EvgeniaMartynova-thebeat · 2021-12-24T16:13:15Z

../client/http/http.go

../client/amqp/v2/amqp.go

../client/es/elasticsearch.go

../client/grpc/grpc.go

../client/redis/redis.go

../client/sns/v2/publisher.go

../client/sql/publisher.go

../client/sns/v2/publisher.go

Added exemplars for the following clients:
../client/http/http.go
../client/amqp/v2/amqp.go
../client/es/elasticsearch.go
../client/grpc/grpc.go
../client/redis/redis.go
../client/sns/v2/publisher.go
../client/sql/publisher.go
../client/sns/v2/publisher.go

Since we use a counter vector in Kafka client we cannot use exemplars there.

mantzas

Components are also missing (Kafka, AWS SQS, etc.). We have only the clients covered.

mantzas · 2021-12-27T15:11:56Z

client/grpc/grpc.go

@@ -4,6 +4,7 @@ package grpc

 import (
 	"context"
+	"github.com/uber/jaeger-client-go"


It seems that the imports are not following the standard layout. check with go fmt or goimports which can be setup in any IDE.

thank you, @mantzas, installed goimports and ran on the changed files

mantzas · 2021-12-27T15:12:56Z

client/grpc/grpc.go

+		durationHistogram := rpcDurationMetrics.WithLabelValues(unary, target, method, rpcStatus.Code().String())
+		if sctx, ok := span.Context().(jaeger.SpanContext); ok {
+			durationHistogram.(prometheus.ExemplarObserver).ObserveWithExemplar(
+				invokeDuration.Seconds(), prometheus.Labels{"traceID": sctx.TraceID().String()},


"traceID" seems to be a good candidate as a constant in the trace package.

created a constant as suggested

run goimports added support of Prometheus exemplars to components http middleware and gRPC observability

…t' into feat/prometheus-exemplars-support

EvgeniaMartynova-thebeat · 2021-12-31T16:01:21Z

Components are also missing (Kafka, AWS SQS, etc.). We have only the clients covered.

introduced Prometheus Exemplars also in
component/http/middleware.go
component/grpc/observability.go

For the other components it seems a server side latency is not measured, do you think it make sense to introduce such metric in this PR for the other components?

mantzas · 2021-12-31T16:59:53Z

other components it

We might not measure latency but we measure message count over time which can be augmented with exemplars in order to give us more metadata. Is this not the case or a good use case? WDYT?

EvgeniaMartynova-thebeat · 2022-01-03T08:26:47Z

other components it

We might not measure latency but we measure message count over time which can be augmented with exemplars in order to give us more metadata. Is this not the case or a good use case? WDYT?

Good point! Just found out that a counter is implementing ExemplarAdder interface, I will try to use it in order to have exemplars for counters.

…tric

EvgeniaMartynova-thebeat · 2022-01-03T15:46:54Z

other components it

We might not measure latency but we measure message count over time which can be augmented with exemplars in order to give us more metadata. Is this not the case or a good use case? WDYT?

Good point! Just found out that a counter is implementing ExemplarAdder interface, I will try to use it in order to have exemplars for counters.

added support of Prometheus Exemplars for request/message CounterVec metric for
component/amqp
component/grpc
component/kafka
component/http/middleware
component/sqs
client/kafka/v2 - partially, because otherwise it would require a change in a signature of exported methods

mantzas

We should only have an empty line in the imports that separate the system from the other packages.

Looking at this and all the other example I am wondering if we should introduce some abstraction to get rid of this repetitive code. Example on the counter: If we introduce a new counter in the trace package that abstracts the prometheus counter we could implemented methods to increase, decrease, add that also take care of the exemplar business.
We have the following option to move into this direction if it makes sense. Make it part of this PR or create a new ticket in Patron to cleanup all of this in a separate PR.

mantzas · 2022-01-03T15:57:15Z

client/amqp/v2/amqp.go

 	"strconv"
 	"time"

+	"github.com/uber/jaeger-client-go"
+


empty line should be removed

mantzas · 2022-01-03T15:57:48Z

client/es/elasticsearch.go

@@ -13,12 +12,14 @@ import (
 	"strings"
 	"time"

+	"github.com/uber/jaeger-client-go"
+


mantzas · 2022-01-03T15:58:28Z

client/grpc/grpc.go

 	"time"

+	"github.com/uber/jaeger-client-go"
+


mantzas · 2022-01-03T15:58:40Z

client/http/http.go

 	"io"
 	"net/http"
 	"strconv"
 	"time"

+	"github.com/uber/jaeger-client-go"
+


mantzas · 2022-01-03T15:59:36Z

client/kafka/v2/kafka.go

+	"github.com/beatlabs/patron/trace"
+	"github.com/opentracing/opentracing-go"
+	"github.com/uber/jaeger-client-go"
+


mantzas · 2022-01-03T16:00:15Z

client/sns/v2/publisher.go

 	"strconv"
 	"time"

+	"github.com/uber/jaeger-client-go"
+


EvgeniaMartynova-thebeat · 2022-01-04T06:57:14Z

We should only have an empty line in the imports that separate the system from the other packages.

Looking at this and all the other example I am wondering if we should introduce some abstraction to get rid of this repetitive code. Example on the counter: If we introduce a new counter in the trace package that abstracts the prometheus counter we could implemented methods to increase, decrease, add that also take care of the exemplar business. We have the following option to move into this direction if it makes sense. Make it part of this PR or create a new ticket in Patron to cleanup all of this in a separate PR.

I also don't like this code duplication, I will do what you are suggesting in this PR.

EvgeniaMartynova-thebeat · 2022-01-07T14:38:44Z

We should only have an empty line in the imports that separate the system from the other packages.
Looking at this and all the other example I am wondering if we should introduce some abstraction to get rid of this repetitive code. Example on the counter: If we introduce a new counter in the trace package that abstracts the prometheus counter we could implemented methods to increase, decrease, add that also take care of the exemplar business. We have the following option to move into this direction if it makes sense. Make it part of this PR or create a new ticket in Patron to cleanup all of this in a separate PR.

I also don't like this code duplication, I will do what you are suggesting in this PR.

Used a decorator pattern to move all repetitive Exemplars related code to trace package. Calling the decorator methods to add/inc/observe metrics.

trace/metric.go

trace/metric_test.go

…t' into feat/prometheus-exemplars-support

siavashs

Nice work @EvgeniaMartynova-thebeat
I just added a minor comment regarding documentation.

siavashs · 2022-01-10T12:35:48Z

docs/observability/Observability.md

+[Prometheus Exemplars](https://grafana.com/docs/grafana/latest/basics/exemplars/)
+it became possible to move seamlessly between metrics, logs, and traces.


I think this can be rephrased as any OpenTracing compatible tracing system (Jaeger and Tempo so far only AFAIK) can work with Prometheus Exemplars, not just Tempo.
The second part regarding seamless integration between metrics, logs and traces is a Grafana specific feature.

rephrased according to the comment, removed the second part.

siavashs

🎖️

teivah · 2022-01-13T08:25:30Z

trace/metric.go

+	spanFromCtx := opentracing.SpanFromContext(ctx)
+	if spanFromCtx != nil {
+		if sctx, ok := spanFromCtx.Context().(jaeger.SpanContext); ok {
+			c.Counter.(prometheus.ExemplarAdder).AddWithExemplar(


Sorry, I'm missing the point about why do we need to make type assertions. If Counter is a prometheus.ExemplarAdder, then why can't we make this field a prometheus.ExemplarAdder,?

A Counter is a prometheus.Counter interface.
Theoretically there could be a bespoke counter implementing prometheus.Counter interface but not prometheus.ExemplarAdder interface, that is why we need a type assertion here.
Or am I missing your point here?

But if we talk about evolutions and that the Counter may be of another type later on, then this code will panic (example). How are we going to handle such a case?

thanks a lot, @teivah, indeed you caught a potential panic, fixed, I applied the same change for Histogram struct in the same file.

teivah · 2022-01-13T08:36:25Z

trace/metric_test.go

+			if tt.expectedPanic {
+				defer func() {
+					if r := recover(); r == nil {
+						t.Errorf("Add method did not panic.")


Suggested change

t.Errorf("Add method did not panic.")

t.Error("Add method did not panic.")

teivah · 2022-01-13T08:36:33Z

trace/metric_test.go

+			if tt.expectedPanic {
+				defer func() {
+					if r := recover(); r == nil {
+						t.Errorf("Add method did not panic.")


Suggested change

t.Errorf("Add method did not panic.")

t.Error("Add method did not panic.")

teivah · 2022-01-13T08:41:50Z

trace/metric_test.go

+	}
+
+	pb := &dto.Metric{}
+	m.Write(pb)


The error isn't handled. I suggest that we ignore it explicitly at least:

Suggested change

m.Write(pb)

_ = m.Write(pb)

teivah · 2022-01-13T08:44:54Z

And also, CI is failing.

EvgeniaMartynova-thebeat added 2 commits December 21, 2021 15:42

add support for Prometheus Exemplars

200bb33

add support for Prometheus Exemplars

587a5d7

EvgeniaMartynova-thebeat requested review from drakos74, mantzas, teivah and tpaschalis as code owners December 21, 2021 14:57

siavashs previously approved these changes Dec 21, 2021

View reviewed changes

mantzas requested changes Dec 21, 2021

View reviewed changes

use opentracing.SpanFromContext() instead of a separate module for no…

e92ba95

…ting TraceID

EvgeniaMartynova-thebeat dismissed siavashs’s stale review via e92ba95 December 21, 2021 15:50

use opentracing.SpanFromContext() instead of a separate module for no…

f183540

…ting TraceID

add support for Prometheus Exemplars for different clients such as gr…

2d3c3e8

…pc, sql, redis, etc; put success label as err == nil for all of the clients

Merge branch 'master' into feat/prometheus-exemplars-support

43002eb

mantzas requested changes Dec 27, 2021

View reviewed changes

EvgeniaMartynova-thebeat added 2 commits December 31, 2021 16:57

create a constant in trace package for TraceID

e9f654d

run goimports added support of Prometheus exemplars to components http middleware and gRPC observability

Merge remote-tracking branch 'origin/feat/prometheus-exemplars-suppor…

40eae9e

…t' into feat/prometheus-exemplars-support

add support of Prometheus Exemplars for request/message CounterVec me…

b19e769

…tric

mantzas requested changes Jan 3, 2022

View reviewed changes

EvgeniaMartynova-thebeat added 2 commits January 7, 2022 15:34

use decoration pattern for metrics to reduce a code duplication

2cee064

use decoration pattern for metrics to reduce a code duplication

cd67c81

mantzas requested changes Jan 7, 2022

View reviewed changes

trace/metric.go Show resolved Hide resolved

trace/metric.go Outdated Show resolved Hide resolved

trace/metric_test.go Outdated Show resolved Hide resolved

Sotirios Mantziaris and others added 3 commits January 7, 2022 18:09

Merge branch 'master' into feat/prometheus-exemplars-support

d4b073c

add documentation, PR comments

a6324be

Merge remote-tracking branch 'origin/feat/prometheus-exemplars-suppor…

cadf753

…t' into feat/prometheus-exemplars-support

mantzas previously approved these changes Jan 10, 2022

View reviewed changes

siavashs reviewed Jan 10, 2022

View reviewed changes

update documentation according to a PR comment

40691fb

EvgeniaMartynova-thebeat dismissed mantzas’s stale review via 40691fb January 10, 2022 14:25

mantzas previously approved these changes Jan 11, 2022

View reviewed changes

mantzas requested a review from siavashs January 11, 2022 08:00

siavashs previously approved these changes Jan 11, 2022

View reviewed changes

teivah reviewed Jan 13, 2022

View reviewed changes

update tests according to a PR comment

a0fad1b

EvgeniaMartynova-thebeat dismissed stale reviews from siavashs and mantzas via a0fad1b January 13, 2022 13:42

EvgeniaMartynova-thebeat and others added 2 commits January 19, 2022 17:39

a potential panic fix

60554ab

Merge branch 'master' into feat/prometheus-exemplars-support

409dede

teivah approved these changes Jan 20, 2022

View reviewed changes

teivah requested a review from mantzas January 20, 2022 08:47

mantzas requested a review from siavashs January 20, 2022 09:06

mantzas approved these changes Jan 20, 2022

View reviewed changes

mantzas merged commit 7d46f2a into beatlabs:master Jan 20, 2022

		[Prometheus Exemplars](https://grafana.com/docs/grafana/latest/basics/exemplars/)
		it became possible to move seamlessly between metrics, logs, and traces.

	t.Errorf("Add method did not panic.")
	t.Error("Add method did not panic.")

Prometheus Exemplars Support #448

Prometheus Exemplars Support #448

Conversation

EvgeniaMartynova-thebeat commented Dec 21, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

mantzas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpaschalis commented Dec 22, 2021

siavashs commented Dec 22, 2021

EvgeniaMartynova-thebeat commented Dec 24, 2021

EvgeniaMartynova-thebeat commented Dec 24, 2021

mantzas left a 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

EvgeniaMartynova-thebeat commented Dec 31, 2021 • edited Loading

mantzas commented Dec 31, 2021

EvgeniaMartynova-thebeat commented Jan 3, 2022

EvgeniaMartynova-thebeat commented Jan 3, 2022

mantzas left a 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

EvgeniaMartynova-thebeat commented Jan 4, 2022

EvgeniaMartynova-thebeat commented Jan 7, 2022

siavashs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siavashs left a 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

Choose a reason for hiding this comment

teivah commented Jan 13, 2022

EvgeniaMartynova-thebeat commented Dec 21, 2021 •

edited

Loading

EvgeniaMartynova-thebeat commented Dec 31, 2021 •

edited

Loading