Fix prom client output #3337

jdoupe · 2017-10-13T02:49:02Z

Required for all PRs:

[X ] Signed CLA.
[X ] Associated README.md updated.
[X ] Has appropriate unit tests.

UPDATE: After the merge of #3351, this PR now only addresses the handling of history and summary types.

This contains a couple fixes for the prometheus_client output.

The prometheus.http module is deprecated. Updated to use prometheus/promhttp.
The plugin renamed metrics by appending field names, even when that didn't make sense.
For example, when using the prometheus input plugin, field names of "counter" and "gauge" were created (instead of just "value"), and then the metrics would have "counter" and "gauge" appended to them on the output.
When the prometheus input was used, the histogram and summary types were not handled gracefully. For histograms, bucket names were appended to the metrics and lost all meaning. For summarys, quantile names were appended to the metrics.

Upon de-mangling the metric names, it became apparent that when using the prometheus input in association with the prometheus output that the metrics created by the prometheus client library were in direct conflict with the same metrics from other scraped prometheus clients. (go_* and process_*). The conflict is not from the metric name itself, but the combination of the metric and the "HELP" text. The internal prometheus client library would generate the metrics with its default (and appropriate) HELP text, and then when a prometheus target was scraped and output by the prometheus client library in the plugin, it gives those same metric names a generic HELP text (because that text isn't saved anywhere in the telegraf data structures). Since the original HELP text doesn't match the generic HELP text for the same metric, the prometheus client library rejects it.

The fix presented is to disable to the collection of the internal client library metrics. They would be a bit conflated with the main telegraf process anyway, and not sure if they'd be directly meaningful.

However, I've put the unregistering of the metrics under the control of a configuration variable, which is false by default so as not to adversely affect existing installations any more than necessary.

danielnelson · 2017-10-13T23:54:56Z

To prevent rejection, does the HELP string need to be the same or would it be enough if the TYPE string was equal?

jdoupe · 2017-10-14T03:53:20Z

Yes, unfortunately the HELP string needs to be the same.

https://github.com/prometheus/client_golang/blob/5cec1d0429b02e4323e042eb04dafdb079ddf568/prometheus/registry.go#L417

danielnelson · 2017-10-18T02:36:26Z

@jdoupe I used your ideas and opened an alternative pull request #3351, it does both more and less than this pull request, but is more in line with how I want to address the issue (using the existing metric value types). There is more info over on that pull request, can you take a look and tell me what you think?

jdoupe · 2017-10-18T14:15:54Z

I agree with your approach, and amazingly I did the same thing yesterday (make the prom input not drop the counter and gauge types) - you beat me to the punch. However, my main goal was to make the histogram and summary types "work" as they are my current problem.

danielnelson · 2017-10-18T21:55:26Z

If you want you can force push your new PR here, or just close this and open a new one.

jdoupe · 2017-10-18T23:08:47Z

Force pushed... yup, needs some clean up. I'll try to get some more time on it later or tomorrow.

jdoupe · 2017-10-19T14:22:58Z

Rebased after merge of #3350...

danielnelson · 2017-10-19T21:07:26Z

accumulator.go

@@ -28,6 +28,18 @@ type Accumulator interface {
 		tags map[string]string,
 		t ...time.Time)

+	// AddCounter is the same as AddFields, but will add the metric as a "Counter" type


Comment needs updated

danielnelson · 2017-10-19T21:07:49Z

accumulator.go

+		tags map[string]string,
+		t ...time.Time)
+
+	// AddCounter is the same as AddFields, but will add the metric as a "Counter" type


Comment needs updated

danielnelson · 2017-10-19T21:17:36Z

plugins/outputs/prometheus_client/prometheus_client.go

@@ -226,6 +254,36 @@ func CreateSampleID(tags map[string]string) SampleID {
 	return SampleID(strings.Join(pairs, ","))
 }

+func (p *PrometheusClient) createFamily(pvt prometheus.ValueType, point telegraf.Metric, sample *Sample, mname string, sampleID SampleID) {


This function doesn't always do what it says it does "createFamily", and its does a lot more. I think it probably needs to be something like func addMetricFamily(name string) MetricFamily and only do the first half and then another function to addSample(.

danielnelson · 2017-10-19T21:18:45Z

plugins/outputs/prometheus_client/prometheus_client.go

@@ -39,7 +45,10 @@ type MetricFamily struct {
 	// Samples are the Sample belonging to this MetricFamily.
 	Samples map[SampleID]*Sample
 	// Type of the Value.
-	ValueType prometheus.ValueType
+	PromValueType prometheus.ValueType


Maybe we can remove the prometheus.ValueType then?

danielnelson · 2017-10-19T21:26:30Z

plugins/outputs/prometheus_client/prometheus_client.go

@@ -242,86 +300,123 @@ func (p *PrometheusClient) Write(metrics []telegraf.Metric) error {
 			labels[sanitize(k)] = v
 		}

-		// Prometheus doesn't have a string value type, so convert string
-		// fields to labels.
-		for fn, fv := range point.Fields() {


I think this loop can stay, since it shouldn't interact with the different types and its theoretically possible to have a string added to a summary.

Side note: This is a symptom of how the ValueType was added later to our metrics, and how the types really should be per field not per metric. We will probably try to redo the Metrics class in the next year but for now we have to deal with it.

danielnelson · 2017-10-19T21:33:36Z

plugins/outputs/prometheus_client/prometheus_client.go

+			for fn, fv := range point.Fields() {
+				switch fn {
+				case "sum":
+					sum = fv.(float64)


We need to guard against this because we don't have full control that it will always be true, and in the other spots where we type assert.

This one really got me. Type assertions suck, and switching on .(type) seemed to be the only solution, but as you noted, we don't have control what the type would be which would result in a pretty long set of cases. After rolling it around awhile (and not being a real in depth go guy), would doing something like strconv.ParseFloat(fmt.Sprint(fv), 64) be too silly or performance degrading? (Convert the value to a string because you don't need to know the type for that, and then convert the string to a float64. If it's not a number, presumably we've already converted it to a label.)

I wouldn't do the string conversion, but you could do the two value return form: f, ok := i.(float64). I guess we need to decide what we are going to do if a field is not a float (do we just drop it?).

You could also try switching over the type instead of the field name, this might end up reading better.

The current functionality is to drop anything but an int64 or float64. (https://github.com/influxdata/telegraf/blob/master/plugins/outputs/prometheus_client/prometheus_client.go#L254)

That seems to have worked thus far, perhaps we just keep going with it until someone finds otherwise. I've gone ahead and made the summary and histogram value handling the same as the gauge and counter handling.

danielnelson · 2017-10-19T21:38:18Z

plugins/outputs/prometheus_client/prometheus_client.go

+			default:
+				metric, err = prometheus.NewConstMetric(desc, family.PromValueType, sample.Value, labels...)
+				if err != nil {
+					log.Printf("E! Error creating prometheus metric, "+


Could share the error handling and log message after the switch

jdoupe · 2017-10-24T15:08:38Z

Tests failed. Wasn't me!
Executing 'make docker-run-circle'
docker run --name aerospike -p "3000:3000" -d aerospike/aerospike-server:3.9.0
Unable to find image 'aerospike/aerospike-server:3.9.0' locally
3.9.0: Pulling from aerospike/aerospike-server

jdoupe · 2017-10-24T16:05:20Z

Working on fixing / adding tests...

jdoupe · 2017-10-24T19:46:36Z

Ready for review once again!

jdoupe force-pushed the fix-prom-client-output branch from ae9866f to 9778252 Compare October 13, 2017 02:59

This was referenced Oct 13, 2017

Type loss and metric rename in prometheus input → prometheus output #2950

Closed

Prometheus output plugin seems to export metric from the input plugin #2327

Closed

danielnelson mentioned this pull request Oct 18, 2017

Fix prometheus passthrough for existing value types #3351

Merged

3 tasks

jdoupe force-pushed the fix-prom-client-output branch from 9778252 to bf7b1a9 Compare October 18, 2017 22:58

jdoupe force-pushed the fix-prom-client-output branch 4 times, most recently from 03c734c to 771832f Compare October 19, 2017 14:20

danielnelson reviewed Oct 19, 2017

View reviewed changes

jdoupe force-pushed the fix-prom-client-output branch from 771832f to 54fc42d Compare October 24, 2017 14:57

jdoupe force-pushed the fix-prom-client-output branch from 54fc42d to e7ce89b Compare October 24, 2017 15:25

jdoupe force-pushed the fix-prom-client-output branch 2 times, most recently from 64c7f7b to cad142c Compare October 24, 2017 19:30

Add history and summary types to telegraf and prometheus plugins

ce88bab

jdoupe force-pushed the fix-prom-client-output branch from cad142c to ce88bab Compare October 24, 2017 19:44

danielnelson added this to the 1.5.0 milestone Oct 24, 2017

danielnelson added the area/prometheus label Oct 24, 2017

danielnelson merged commit a6797a4 into influxdata:master Oct 24, 2017

jdoupe deleted the fix-prom-client-output branch October 25, 2017 01:22

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add history and summary types to telegraf and prometheus plugins (#3337)

43d69d8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prom client output #3337

Fix prom client output #3337

jdoupe commented Oct 13, 2017 •

edited

Loading

danielnelson commented Oct 13, 2017

jdoupe commented Oct 14, 2017

danielnelson commented Oct 18, 2017

jdoupe commented Oct 18, 2017

danielnelson commented Oct 18, 2017

jdoupe commented Oct 18, 2017

jdoupe commented Oct 19, 2017

danielnelson Oct 19, 2017

danielnelson Oct 19, 2017

danielnelson Oct 19, 2017

danielnelson Oct 19, 2017

danielnelson Oct 19, 2017

danielnelson Oct 19, 2017

jdoupe Oct 20, 2017

danielnelson Oct 21, 2017

jdoupe Oct 24, 2017

danielnelson Oct 19, 2017

jdoupe commented Oct 24, 2017

jdoupe commented Oct 24, 2017

jdoupe commented Oct 24, 2017

Fix prom client output #3337

Fix prom client output #3337

Conversation

jdoupe commented Oct 13, 2017 • edited Loading

Required for all PRs:

danielnelson commented Oct 13, 2017

jdoupe commented Oct 14, 2017

danielnelson commented Oct 18, 2017

jdoupe commented Oct 18, 2017

danielnelson commented Oct 18, 2017

jdoupe commented Oct 18, 2017

jdoupe commented Oct 19, 2017

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

jdoupe commented Oct 24, 2017

jdoupe commented Oct 24, 2017

jdoupe commented Oct 24, 2017

jdoupe commented Oct 13, 2017 •

edited

Loading