Prometheus exporter #334

paivagustavo · 2019-11-19T21:40:14Z

This adds prometheus metrics exporter.

This was started by @shreyassrivatsan and I've updated it with the last updates of the metrics sdk and support for measures (histogram and summary).

jmacd

This is a good start! Thanks @paivagustavo. FYI @shreyassrivatsan we've taken this over, thanks for the initial draft and we'll keep you in the loop.

example/prometheus/main.go

exporter/metric/prometheus/prometheus.go

jmacd · 2019-11-20T00:21:09Z

exporter/metric/prometheus/prometheus.go

+	return tagKeys
+}
+
+func getCanonicalID(desc *export.Descriptor, labels export.Labels) (string, []string) {


The way this is written, we need to use the defaultkeys Batcher, which ensures that the labels on each export Record are the same. I think this is another reason to integrate the exporter with the controller (and let it construct a Batcher).

The defaultkeys Batcher will let us simplify this code, since we can be fairly sure that the calls to With() will match the tags declared for the vector.

This method is not needed anymore so I have just deleted.

Do you mean this because of the labels.Encoded() call? because I've just moved it to another place.

I guess I should make the Prometheus exporter implements the LabelEncoder with a defaultkeys batcher for the moment?

My understanding is that no two -Vec types can exist with the same name and different key sets. The defaultkeys batcher will ensure that the tag sets are always equal--that's why it makes sense as a default. I still envision other batchers being created to support greater configuration, but for now yes, I would use defaultkeys.

Also, yes, it makes sense to use the LabelEncoder interface to compute anything needed to map the -Vec type into the handle-like interface. If the map[string]string input to prometheus can be changed to a slice of values (as you said it could), then maybe the Encode() function will compute a slice of values.

exporter/metric/prometheus/prometheus.go

exporter/metric/prometheus/sanitize.go

exporter/metric/prometheus/prometheus.go

exporter/metric/prometheus/sanitize.go

jmacd

Looking good!

exporter/metric/prometheus/sanitize.go

example/prometheus/main.go

exporter/metric/prometheus/prometheus.go

shreyassrivatsan · 2019-11-21T07:06:49Z

exporter/metric/prometheus/prometheus.go

+			encoded: record.Labels().Encoded(),
+		}
+
+		// TODO(paivagustavo): how to choose between Histogram and Summary?


I think for now this can be in the exporter config. The user decides between histogram and summary at construction time.

In the future it would be nice to give a way to configure this on a per metric basis, but I am not very certain how that would looke..

Yes, I'm interested in more flexible and configurable exporters. I believe there's a question as to whether such a configuration can be in any way shared by more than one exporter.

I've added an option to change how measures are exported, defaulting to histograms.

exporter/metric/prometheus/prometheus.go

shreyassrivatsan · 2019-11-21T07:10:35Z

exporter/metric/prometheus/prometheus.go

+}
+
+func (e *Exporter) getCounterVec(desc *export.Descriptor, labels export.Labels) (*prometheus.CounterVec, error) {
+	if c, ok := e.counterVecs[desc]; ok {


maybe a separate loc for the *Vec methods?

Sorry, I actually didn't understand your suggestion here, could you discuss a little further?

exporter/metric/prometheus/sanitize.go

exporter/metric/prometheus/prometheus.go

…lEncoder'

shreyassrivatsan · 2019-11-22T19:19:03Z

exporter/metric/prometheus/counter.go

+	counterVecs map[*export.Descriptor]*prometheus.CounterVec
+}
+
+func newCounters(registerer prometheus.Registerer) counters {


i like this refactor..

this should probably return a pointer, no? the methods on the object rightly have a pointer receiver, so might as well return a pointer and reduce a copy here..

same applies to histograms and gauges too..

These are placed in structs, so the pointer methods work. I'm happy with fewer pointers, fwiw.

We could reduce a copy using pointer or save an alloc not using a pointer. In the end both should have little to no difference since exporters are created once.

Are you okay leaving this that way or do you have a strong objections against this?

Let's leave this. In the Go base libraries, this sort of class is often accompanied by a "Must not be copied after first use" sort of comment--would that help?

I actually don't know if that solves it. Copies could be done, it would just be a shallow copy, It is not like Mutex that a copy could have inconsistent and incorrect state.

I don't see a problem, anyway.

exporter/metric/prometheus/sanitize.go

paivagustavo · 2019-11-24T05:33:42Z

Thanks @shreyassrivatsan and @jmacd, for helping and reviewing this during the development. I've responded to all the open discussions.

I think we can now open this for others to review.

jmacd · 2019-11-25T17:53:32Z

example/prometheus/main.go

+)
+
+func initMeter() *push.Controller {
+	selector := simple.NewWithExactMeasure()


I'd like to create a simple wrapper for this block of code, as mentioned in #345. It's OK if you think we should wait for a future PR.

I think we should take that on a next PR, I would happily take it when this gets merged.

jmacd · 2019-11-25T17:54:17Z

exporter/metric/prometheus/counter.go

+	counterVecs map[*export.Descriptor]*prometheus.CounterVec
+}
+
+func newCounters(registerer prometheus.Registerer) counters {


Let's leave this. In the Go base libraries, this sort of class is often accompanied by a "Must not be copied after first use" sort of comment--would that help?

ferhatelmas · 2019-11-25T22:34:00Z

exporter/metric/prometheus/prometheus.go

+// Export exports the provide metric record to prometheus.
+func (e *Exporter) Export(_ context.Context, checkpointSet export.CheckpointSet) error {
+	var forEachError error
+	checkpointSet.ForEach(func(record export.Record) {


isn't a guard required if forEachError is nil or not ?

We've being following the thought that if a single record fails to be exported that should not prevent the rest of the records to be processed.

There is still some questions we need to figure out about how to handle errors better on this ForEach method. One of the suggestions is to change its argument from func(export.Record) to func (export.Record) error to have a more uniform and simpler error handling on all exporters. But that is an open question that I think should be taken on a separate PR since it would touch not only this exporter but others too.

rghetia · 2019-11-26T18:51:41Z

@paivagustavo, I merged it to master but it failed due to #356 . Can you please fix the build? Thx.

paivagustavo · 2019-11-26T19:36:53Z

@rghetia just fixed it :)

shreyassrivatsan and others added 2 commits November 18, 2019 23:22

Prom exporter structure

2d65e1f

update prometheus exporter with master and add example.

f5e989f

jmacd reviewed Nov 20, 2019

View reviewed changes

shreyassrivatsan reviewed Nov 20, 2019

View reviewed changes

paivagustavo added 12 commits November 19, 2019 21:52

remove distributedcontext from prometheus example

f6e48f5

docs and interface checker

b6dccb9

make precommit

0faa669

make precommit & remove "OnRegisterError"

4e01fd1

coerce values to float

3f1cb60

return register errors and maybe fix precommit?

1bf4b56

Merge remote-tracking branch 'upstream/master' into prometheus_exporter

856c676

add option to specify a prometheus.Registry

0520022

make exporter implement http.Handler interface

736f9f5

fix map keys bugs

4edfbea

remove unused const

5481310

fix modules dependencies.

9a45478

jmacd reviewed Nov 20, 2019

View reviewed changes

exporter/metric/prometheus/sanitize.go Show resolved Hide resolved

This was referenced Nov 20, 2019

WIP: Initial implementation of prometheus exporter #296

Closed

Dogstatsd metrics exporter #326

Merged

paivagustavo added 3 commits November 21, 2019 01:28

add support for histogram

731794e

get metrics with labels values only instead of a labels map

7ad447d

make exporter implements label encoder interface

50c4fdf

shreyassrivatsan reviewed Nov 21, 2019

View reviewed changes

encode labels if the encoder is different.

ab92d1b

jmacd reviewed Nov 22, 2019

View reviewed changes

exporter/metric/prometheus/prometheus.go Show resolved Hide resolved

paivagustavo added 5 commits November 22, 2019 02:19

split metrics on several files and encapsulate them in structs

38a2bee

make pre commit

3aad483

unexport 'sanitize'

2e11207

Merge remote-tracking branch 'upstream/master' into prometheus_exporter

7a02dfc

remove 'AllValues' in favor of 'Points' and change to 'NewDefaultLabe…

bc74d6f

…lEncoder'

shreyassrivatsan approved these changes Nov 22, 2019

View reviewed changes

paivagustavo added 5 commits November 23, 2019 21:14

add todos on sanitize

1b71f48

add support for summaries. custom remove label encoder.

dcbf80c

imports

de66b42

Merge remote-tracking branch 'upstream/master' into prometheus_exporter

5597aca

imports

d131ad1

paivagustavo marked this pull request as ready for review November 24, 2019 05:33

paivagustavo requested review from iredelmeier, lizthegrey, rghetia, sjkaris and tedsuo as code owners November 24, 2019 05:33

paivagustavo added area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package labels Nov 24, 2019

jmacd approved these changes Nov 25, 2019

View reviewed changes

ferhatelmas reviewed Nov 25, 2019

View reviewed changes

Merge branch 'master' into prometheus_exporter

30465b0

rghetia approved these changes Nov 26, 2019

View reviewed changes

Merge branch 'master' into prometheus_exporter

7f27e4c

update with upstream

273f12d

rghetia merged commit 3d78564 into open-telemetry:master Nov 26, 2019

paivagustavo deleted the prometheus_exporter branch November 26, 2019 20:18

This was referenced Dec 4, 2019

Prometheus metrics exporter #115

Closed

Prometheus client should not be used to aggregate metrics points #370

Closed

c24t mentioned this pull request Dec 20, 2019

Add Prometheus exporter open-telemetry/opentelemetry-python#157

Closed

hstan referenced this pull request in hstan/opentelemetry-go Oct 15, 2020

Restore tests for auth_test.go on 386 (#334)

3ea5332

pellared added this to the untracked milestone Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus exporter #334

Prometheus exporter #334

paivagustavo commented Nov 19, 2019 •

edited

Loading

jmacd left a comment

jmacd Nov 20, 2019

paivagustavo Nov 20, 2019

jmacd Nov 20, 2019

jmacd left a comment

shreyassrivatsan Nov 21, 2019

jmacd Nov 22, 2019

paivagustavo Nov 24, 2019

shreyassrivatsan Nov 21, 2019

paivagustavo Nov 24, 2019

shreyassrivatsan Nov 22, 2019

jmacd Nov 22, 2019

paivagustavo Nov 24, 2019

jmacd Nov 25, 2019

paivagustavo Nov 26, 2019

jmacd Nov 26, 2019

paivagustavo commented Nov 24, 2019

jmacd Nov 25, 2019

paivagustavo Nov 26, 2019

jmacd Nov 25, 2019

ferhatelmas Nov 25, 2019

paivagustavo Nov 26, 2019

rghetia commented Nov 26, 2019 •

edited

Loading

paivagustavo commented Nov 26, 2019

Prometheus exporter #334

Prometheus exporter #334

Conversation

paivagustavo commented Nov 19, 2019 • edited Loading

jmacd 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

jmacd 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

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

paivagustavo commented Nov 24, 2019

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

rghetia commented Nov 26, 2019 • edited Loading

paivagustavo commented Nov 26, 2019

paivagustavo commented Nov 19, 2019 •

edited

Loading

rghetia commented Nov 26, 2019 •

edited

Loading