exporter/prometheusexporter: migrate code in here #479

odeke-em · 2019-03-07T19:19:52Z

Hand over prometheus exporter code that I had parked
in https://github.com/orijtech/prometheus-go-metrics-exporter

That code was written for OpenCensus Authors, and was just
waiting for a final destination. This change finishes that.

Fixes #478

codecov · 2019-03-07T19:29:46Z

Codecov Report

Merging #479 into master will increase coverage by 1.4%.
The diff coverage is 88.65%.

@@            Coverage Diff            @@
##           master     #479     +/-   ##
=========================================
+ Coverage   57.02%   58.43%   +1.4%     
=========================================
  Files          69       71      +2     
  Lines        4247     4439    +192     
=========================================
+ Hits         2422     2594    +172     
- Misses       1666     1682     +16     
- Partials      159      163      +4

Impacted Files	Coverage Δ
exporter/prometheusexporter/prometheus.go	`74.28% <100%> (ø)`	⬆️
exporter/prometheusexporter/sanitize.go	`100% <100%> (ø)`
exporter/prometheusexporter/exporter.go	`87.57% <87.57%> (ø)`
...al/collector/processor/nodebatcher/node_batcher.go	`79.09% <0%> (+0.9%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51944b6...99046b1. Read the comment docs.

pjanotti

Hi @odeke-em, nice to see you getting this to the repo. I'm not done with the review but since I already had some Qs and comments, let's get the ball rolling.

exporter/prometheusexporter/sanitize.go

exporter/prometheusexporter/exporter.go

Hand over prometheus exporter code that I had parked in https://github.com/orijtech/prometheus-go-metrics-exporter That code was written for OpenCensus Authors, and was just waiting for a final destination. This change finishes that. Fixes #478

Bring in sanitize_test.go from OpenCensus-Go.

odeke-em · 2019-03-12T17:41:19Z

Thank you for the review @pjanotti! I've addressed your feedback, PTAL.

songy23 · 2019-03-12T22:22:41Z

exporter/prometheusexporter/exporter.go

+		buckets := make([]float64, 0, len(dValue.Buckets))
+		for index, bucket := range dBuckets {
+			if _, added := indicesMap[bucket]; !added {
+				indicesMap[bucket] = index


Agree with @pjanotti, I don't think the check and sort is necessary here. In Proto we already specified bucket bounds are in strictly increasing order.

songy23 · 2019-03-12T22:25:12Z

exporter/prometheusexporter/exporter.go

+
+		points := make(map[float64]uint64, len(buckets))
+		for _, bucket := range buckets {
+			index := indicesMap[bucket]


Same here, I don't think the reverse lookup is necessary.

exporter/prometheusexporter/exporter.go

exporter/prometheusexporter/sanitize.go

pjanotti · 2019-03-13T19:53:40Z

exporter/prometheusexporter/exporter.go

+		buckets := make([]float64, 0, len(dValue.Buckets))
+		for index, bucket := range dBuckets {
+			if _, added := indicesMap[bucket]; !added {
+				indicesMap[bucket] = index


No question that for the Prometheus API used here the histograms need to be cumulative. This is not about making it cumulative for Prometheus but how far we go to support data that is malformed. Giving that code in the service should be in charge of converting to proto before this point it seems that the code gives support to bad receivers.

pjanotti · 2019-03-13T23:23:43Z

exporter/prometheusexporter/exporter_test.go

+										Count:                 1,
+										Sum:                   11.9,
+										SumOfSquaredDeviation: 0,
+										Buckets: []*metricspb.DistributionValue_Bucket{


Per proto spec this is not valid proto: you should have one more entry in the Buckets field then what you have on the Bounds

pjanotti · 2019-03-13T23:43:11Z

exporter/prometheusexporter/exporter.go

+
+func (c *collector) Describe(ch chan<- *prometheus.Desc) {
+	c.registeredMetricsMu.Lock()
+	registered := make(map[string]*prometheus.Desc)


nit: map capacity hint, yes, it is just a hint but one expects the standard library to take advantage of that (otherwise why implement it :)

allocate the map outside the lock

pjanotti · 2019-03-14T16:30:29Z

exporter/prometheusexporter/exporter.go

+	}
+}
+
+var _ http.Handler = (*Exporter)(nil)


nit: move close to Exporter type.

pjanotti · 2019-03-14T16:37:55Z

exporter/prometheusexporter/exporter.go

+	return desc, signature, ok
+}
+
+func (c *collector) registerMetrics(metrics ...*metricspb.Metric) error {


nit: the only usage of this function passes a single *metricspb.Metric so there is no need for the variadic argument.

nit: on the usage site of registerMetrics is always rebuilds the signature, consider returning it.

pjanotti · 2019-03-14T16:48:19Z

exporter/prometheusexporter/exporter.go

+				protoLabelKeysToLabels(metric.GetMetricDescriptor().GetLabelKeys()),
+				c.opts.ConstLabels,
+			)
+			c.registeredMetricsMu.Lock()


Most of the time we the lookup above should succeed, putting that together with the copy that needs to happen from time to time, it seems that a reader-writer mutex will be more appropriate.

pjanotti · 2019-03-14T16:57:24Z

exporter/prometheusexporter/exporter.go

+		return nil, err
+	}
+	derivedPrometheusValueType := prometheusValueType(metric)
+	desc, _, _ := c.lookupPrometheusDesc(c.opts.Namespace, metric)


Is it guaranteed that the metric signature was already added?

pjanotti · 2019-03-14T17:55:37Z

exporter/prometheusexporter/exporter.go

+func (c *collector) ensureRegisteredOnce() error {
+	var finalErr error
+	c.registerOnce.Do(func() {
+		if err := c.registry.Register(c); err != nil {


q.: are the costs of simply registering at construction too high? what are the downsides if registered and no metric ever registered?

songy23

Please migrate the code to https://github.com/census-ecosystem/opencensus-go-exporter-prometheus instead.

odeke-em · 2019-06-04T17:10:35Z

Sorry for the late reply @songy23! I have been swamped with work. Sure we can definitely move it over there but I'll get free cycles perhaps in the next 36 hours to move that over to the new repo. I shall close this PR and add an action item for myself.

songy23 · 2019-06-04T17:13:38Z

No worries, I filed census-ecosystem/opencensus-go-exporter-prometheus#11 to keep track of that.

odeke-em requested review from bogdandrutu, sjkaris, pjanotti and songy23 March 7, 2019 19:19

odeke-em requested a review from a team as a code owner March 7, 2019 19:19

googlebot added the cla: yes label Mar 7, 2019

flands added this to the 0.1.4 milestone Mar 7, 2019

pjanotti reviewed Mar 8, 2019

View reviewed changes

odeke-em added 3 commits March 12, 2019 10:03

exporter/prometheusexporter: bring sanitize_test.go in

b8a3eef

Bring in sanitize_test.go from OpenCensus-Go.

exporter/prometheusexporter: address some PR feedback

7464dc5

exporter/prometheusexporter: revert registry decoupling

99046b1

songy23 reviewed Mar 12, 2019

View reviewed changes

odeke-em mentioned this pull request Mar 13, 2019

Upgrade opencensus-proto version to v0.2.0. #490

Merged

flands modified the milestones: 0.1.4, 0.2.0 Mar 14, 2019

pjanotti reviewed Mar 14, 2019

View reviewed changes

flands modified the milestones: 0.1.5, 0.1.6 Mar 28, 2019

flands removed this from the 0.1.6 milestone Apr 24, 2019

songy23 reviewed Jun 3, 2019

View reviewed changes

odeke-em closed this Jun 4, 2019

odeke-em deleted the bring-in-prometheus-metrics-exporter branch June 4, 2019 17:10

songy23 mentioned this pull request Jun 4, 2019

Implement ExportMetricProto and import code from prometheus-go-metrics-exporter. census-ecosystem/opencensus-go-exporter-prometheus#11

Open

flands added this to the 0.1.8 milestone Jun 5, 2019

flands assigned odeke-em Jun 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exporter/prometheusexporter: migrate code in here #479

exporter/prometheusexporter: migrate code in here #479

odeke-em commented Mar 7, 2019

codecov bot commented Mar 7, 2019 •

edited

Loading

pjanotti left a comment

odeke-em commented Mar 12, 2019

songy23 Mar 12, 2019

songy23 Mar 12, 2019

pjanotti Mar 13, 2019

pjanotti Mar 13, 2019

pjanotti Mar 13, 2019

pjanotti Mar 14, 2019

pjanotti Mar 14, 2019

pjanotti Mar 14, 2019

pjanotti Mar 14, 2019

pjanotti Mar 14, 2019

songy23 left a comment

odeke-em commented Jun 4, 2019

songy23 commented Jun 4, 2019

exporter/prometheusexporter: migrate code in here #479

exporter/prometheusexporter: migrate code in here #479

Conversation

odeke-em commented Mar 7, 2019

codecov bot commented Mar 7, 2019 • edited Loading

Codecov Report

pjanotti left a comment

Choose a reason for hiding this comment

odeke-em commented Mar 12, 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

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

songy23 left a comment

Choose a reason for hiding this comment

odeke-em commented Jun 4, 2019

songy23 commented Jun 4, 2019

codecov bot commented Mar 7, 2019 •

edited

Loading