Switch to Prometheus decoders for Prometheus custom metric endpoint parsing #1473

jimmidyson · 2016-09-20T11:59:57Z

@fabxc This is a hand-rolled parser to collect application metrics from containers. Forgetting the question or application metrics & cadvisor, I assume the better approach for this would be to use Prometheus' own parser? If so, can you point me to it please?

k8s-bot · 2016-09-20T12:02:41Z

Jenkins GCE e2e

Build/test failed for commit e3f72e9.

Build Log

jimmidyson · 2016-09-20T12:18:27Z

mkdir: cannot create directory ‘/tmp/cadvisor-768’: No space left on device - @timstclair any chance you could sort that out?

fabxc · 2016-09-20T12:47:17Z

For reference, the parser is located in https://github.com/prometheus/common/tree/master/expfmt

jimmidyson · 2016-09-20T16:51:45Z

@fabxc Would be you be able to take a look if you have some time? Switched to expfmt decoders & seems to work nicely (few tweaks to tests but nothing fundamental changing in functionality).

k8s-bot · 2016-09-20T16:52:34Z

Jenkins GCE e2e

Build/test failed for commit 80e3f7b.

Build Log

timstclair · 2016-09-20T18:37:54Z

@jimmidyson I'm aware of the issue. I'll try to have it sorted out by EOD

timstclair · 2016-09-20T19:37:59Z

@k8s-bot test this

k8s-bot · 2016-09-20T19:38:34Z

Jenkins GCE e2e

Build/test failed for commit 80e3f7b.

Build Log

k8s-bot · 2016-09-20T20:18:26Z

Jenkins GCE e2e

Build/test passed for commit d5c95e3.

Build Log

fabxc · 2016-09-21T13:05:51Z

collector/prometheus_collector.go

-				FloatValue: metVal,
-				Timestamp:  currentTime,
+				FloatValue: float64(sample.Value),
+				Timestamp:  sample.Timestamp.Time(),
 			}


Not sure I entirely understand this. We seem to ignore the labels of the samples entire and just append timestamp/value pairs for the metric name?

Ah yeah I forgot to ask what the purpose of this label was. Considering there are likely to be multiple labels on each metric, what should this be set to? @timstclair?

@timstclair @DirectXMan12 What should the value of label be in this case? A concatenation of all Prometheus labels?

A concatenation of all Prometheus labels?

Yeah, seems like it. That seems... poor. It works ok for one label, but multiple labels could come in any order. If we wanted to make sure this was comparable, we could sort them first. Doesn't look like the label is actually carried through to the Kube summary API.

Yeah, I agree this is not ideal. Unfortunately this requires an API change to fix properly. Can you file an issue? Sorting + concatenating sounds good for now. Check the allowed symbols on prometheus labels to decide how to concatenate.

In label values all unicode runes are allowed. In Prometheus itself we hence us \0xff as an invalid rune as a separator for unambiguous concatenation.

Hope what I've done it right... always confused by unicode stuff like this. Added tests too.

timstclair · 2016-09-21T18:28:15Z

collector/prometheus_collector.go

 	"time"

+	dto "github.com/prometheus/client_model/go"


what does dto stand for?

Data transfer object. Kinda nasty, but better than go and kinda what we use everywhere else in Prometheus land.

none-the-less, something more expressive might be in order (perhaps rawmodel or rawtypes?)

timstclair · 2016-09-21T18:31:31Z

collector/prometheus_collector.go

-	for _, line := range lines {
-		if line == "" {
+	var (
+		decSamples = make(model.Vector, 0, 50)


How'd you pick 50? What happens if this overflows?

It's just the initial capacity. The slice is appended to, so it just allocates more if we go beyond.
50 SGTM. It's a number of metrics we can expect from virtually any endpoint, grow allocations beyond that are fine. But we basically save 6 re-allocations for the general case.

Plus, the slice is just reset at the end and re-used.

you should probably add a comment noting that it's just a guess at a useful initial capacity

DirectXMan12

while you're at it, the affected functions should probably have proper godoc.

DirectXMan12 · 2016-09-22T18:28:34Z

collector/prometheus_collector.go

+	case dto.MetricType_GAUGE:
+		return v1.MetricGauge
+	default:
+		return v1.MetricType(t.String())


uhh..., if people are expecting this to be only one of these set values, this is going to throw them for a loop.
v1.MetricType seems to be an implicit enumeration (since Go doesn't have explicit enumerations :-/), so I really don't think it's a good idea to arbitrarily add extra values to the enum at runtime.

Types aren't arbitrary, it's just that cadvisor doesn't support the same metric types as Prometheus & there's no way to map e.g. a Prometheus histogram to a cadvisor metric type.

Still not sure how to handle this nicely... This PR doesn't fix the existing problem, but doesn't make it worse so will raise a separate issue to figure out how to handle it, but assume that to be in a separate PR.

DirectXMan12 · 2016-09-22T18:32:37Z

collector/prometheus_collector.go

 		}
+		if _, ok := collector.metricsSet[name]; collector.metricsSet != nil && !ok {


this could really use a comment to indicate why it's checking this (e.g. // skip any metrics not on our list to collect). I realize that the old code doesn't have a comment, but we should fix that (I blame myself :-P).

DirectXMan12 · 2016-09-22T18:38:17Z

collector/prometheus_collector.go

-	for _, line := range lines {
-		if line == "" {
+	var (
+		decSamples = make(model.Vector, 0, 50)


you should probably add a comment noting that it's just a guess at a useful initial capacity

DirectXMan12 · 2016-09-22T18:41:22Z

collector/prometheus_collector.go

-				FloatValue: metVal,
-				Timestamp:  currentTime,
+				FloatValue: float64(sample.Value),
+				Timestamp:  sample.Timestamp.Time(),
 			}


A concatenation of all Prometheus labels?

Yeah, seems like it. That seems... poor. It works ok for one label, but multiple labels could come in any order. If we wanted to make sure this was comparable, we could sort them first. Doesn't look like the label is actually carried through to the Kube summary API.

DirectXMan12 · 2016-09-22T18:49:46Z

collector/prometheus_collector.go

 	"time"

+	dto "github.com/prometheus/client_model/go"


none-the-less, something more expressive might be in order (perhaps rawmodel or rawtypes?)

k8s-bot · 2016-09-23T11:58:38Z

Jenkins GCE e2e

Build/test passed for commit 078eed5.

Build Log

…visor

k8s-bot · 2016-09-23T12:10:36Z

Jenkins GCE e2e

Build/test passed for commit 923dbc5.

Build Log

jimmidyson · 2016-09-23T12:33:28Z

Ready for a second round review.

DirectXMan12

I think this LGTM.

Some of the work that this PR is doing is reversing or working around the work done by the SampleDecoder (e.g. the SampleDecoder converts labels from pairs to a map, and then we convert back to pairs, and the whole "placing samples into a slice and then into a map" is a bit of extra overhead). Unfortunately, just using the Decoder directly requires a decent bit of boilerplate, so the code in this PR seems reasonable.

DirectXMan12 · 2016-09-23T14:25:18Z

collector/prometheus_collector.go

+	case rawmodel.MetricType_GAUGE:
+		return v1.MetricGauge
+	default:
+		return v1.MetricType(t.String())


Please file a follow-up issue about issue about this, because people probably won't expect to see any values beyond what's defined in v1 (whether that means defining more types, or an "other" type, or something else)

jimmidyson · 2016-09-26T11:14:22Z

@timstclair What's left to get this merged?

timstclair · 2016-09-26T18:44:39Z

LGTM, feel free to merge.

jimmidyson force-pushed the prom-collector-label-spaces branch from e3f72e9 to 80e3f7b Compare September 20, 2016 16:50

jimmidyson force-pushed the prom-collector-label-spaces branch from 80e3f7b to d5c95e3 Compare September 20, 2016 20:14

jimmidyson changed the title ~~Handle spaces in Prometheus labels~~ Switch to Prometheus decoders for Prometheus custom metric endpoint parsing Sep 21, 2016

fabxc reviewed Sep 21, 2016

View reviewed changes

timstclair reviewed Sep 21, 2016

View reviewed changes

DirectXMan12 suggested changes Sep 22, 2016

View reviewed changes

Switch to Prometheus decoder

041c5af

jimmidyson force-pushed the prom-collector-label-spaces branch from d5c95e3 to 078eed5 Compare September 23, 2016 11:55

Convert Prometheus labels into single consistent string label for cad…

923dbc5

…visor

jimmidyson force-pushed the prom-collector-label-spaces branch from 078eed5 to 923dbc5 Compare September 23, 2016 12:06

DirectXMan12 approved these changes Sep 23, 2016

View reviewed changes

timstclair added the release-note label Sep 26, 2016

timstclair approved these changes Sep 26, 2016

View reviewed changes

jimmidyson merged commit 0c6b72d into google:master Sep 26, 2016

		}
		if _, ok := collector.metricsSet[name]; collector.metricsSet != nil && !ok {

Switch to Prometheus decoders for Prometheus custom metric endpoint parsing #1473

Switch to Prometheus decoders for Prometheus custom metric endpoint parsing #1473

Conversation

jimmidyson commented Sep 20, 2016

k8s-bot commented Sep 20, 2016

jimmidyson commented Sep 20, 2016

fabxc commented Sep 20, 2016

jimmidyson commented Sep 20, 2016

k8s-bot commented Sep 20, 2016

timstclair commented Sep 20, 2016

timstclair commented Sep 20, 2016

k8s-bot commented Sep 20, 2016

k8s-bot commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc Sep 22, 2016 • edited Loading

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

fabxc Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 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

DirectXMan12 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Sep 23, 2016

k8s-bot commented Sep 23, 2016

jimmidyson commented Sep 23, 2016

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmidyson commented Sep 26, 2016

timstclair commented Sep 26, 2016

DirectXMan12 Sep 22, 2016 •

edited

Loading

fabxc Sep 22, 2016 •

edited

Loading

fabxc Sep 21, 2016 •

edited

Loading

DirectXMan12 Sep 22, 2016 •

edited

Loading

DirectXMan12 Sep 22, 2016 •

edited

Loading