Very preliminary OpenMetrics creation #219

beorn7 · 2020-01-18T23:02:35Z

This is mostly to enable exemplar rendering in client_golang.

The blacklisting feature is introduced here so that OpenMetrics can be configured as opt-in in client_golang.

expfmt/encode.go

expfmt/openmetrics_create.go

expfmt/openmetrics_create_test.go

expfmt/openmetrics_create.go

expfmt/encode.go

brian-brazil · 2020-01-19T07:05:29Z

expfmt/openmetrics_create_test.go

@@ -36,7 +36,7 @@ func TestCreateOpenMetrics(t *testing.T) {
 		in  *dto.MetricFamily
 		out string
 	}{
-		// 0: Counter, NaN as value, timestamp given.
+		// 0: Counter, NaN as value, timestamp given, no _total suffix.


A counter can't have a NaN value.

Is that part of the OpenMetrics spec or generally true?

OpenMetrics spec, also generally true (but not enforced).

There's also limits on the size of the exemplar labels, which should probably also be implemented somewhere client side at some point so the whole scrape doesn't get dropped.

I don't mind us not special casing it, but our tests should be for valid output.

OK, so I have done the following now:

Changed the test.

Added to the doc comment of MetricFamilyToOpenMetrics that size of exemplar labels and the value of a counter isn't checked yet.

brian-brazil · 2020-01-19T07:07:17Z

It occured to me that we're going to need to change the le/quantile values for the Prometheus format at this point too, so you get the same series no matter what is negotiated under the covers.

beorn7 · 2020-01-19T14:59:27Z

It occured to me that we're going to need to change the le/quantile values for the Prometheus format at this point too, so you get the same series no matter what is negotiated under the covers.

I'd say the opposite is required: We don't want to change the le/quantile labels as long as the Prometheus format is negotiated. It would be a breaking change of client_golang if we did. (We mitigated the impact by changing the histogram_quantile function, but it will still break many use cases.)

Only changing the le/quantile labels when negotiating OpenMetrics is at least a change that will only happen if a config is changed (in client_golang, OpenMetrics negatiation has to be activated).

It's still not ideal but better than changing the format by a minor version upgrade in client_golang.

Also, I'm still hopeful that OpenObservability/OpenMetrics#129 makes it into the spec. Then we don't need to worry in OpenMetrics.

(And if OpenObservability/OpenMetrics#129 didn't make it into the spec, we wouldn't know an algorithm to reproducibly create correct OM, so we don't have to worry, either.)

brian-brazil

I'm still hopeful that OpenObservability/OpenMetrics#129 makes it into the spec.

That's not looking likely as it stands. At most we'll make it a SHOULD rather than MUST to produce the right output (it remains an open question), and client_golang should produce that output in either case.

in client_golang, OpenMetrics negatiation has to be activated

I'd hope we'd be enabling it be default, what are your plans there?

we wouldn't know an algorithm to reproducibly create correct OM, so we don't have to worry, either

Such algorithms are well known, and implemented in many languages including Go.

beorn7 · 2020-01-19T17:11:48Z

At most we'll make it a SHOULD rather than MUST to produce the right output

But nobody knows how to create the right output.

I'd hope we'd be enabling it be default, what are your plans there?

At the moment, I cannot see a way how to do it without breaking users in more or less subtle ways. Given the widespread use of the library, I don't think we should make it the default without a major version bump. Thus, I'd like to keep it an opt-in in v1.

Such algorithms are well known, and implemented in many languages including Go.

My understanding is, as explained before, that all algorithms we know are concerned with reproducibly creating the same float64 from a given string rather then creating the same string from a given float64. But for OM we need the latter. Happy to be proven wrong, but that hasn't happened yet.

Signed-off-by: beorn7 <beorn@grafana.com>

beorn7 · 2020-01-19T17:21:01Z

To summarize: My goal here was to let us experiment with exemplars and OM without breaking existing users of this code (and neither client_golang). As the discussion above lead to the insight that an uninformed upgrade of prometheus/common could trigger OM negotiation without the user being aware that the # EOF line has to be added (one way or the other), that seems impossible to accomplish.

To avoid working from a branch, I'll add another commit to this PR that uses a separate Negotiate function for OM. We can then decide if we want to merge that or keep working from a branch for the experiments.

brian-brazil · 2020-01-19T17:34:40Z

Godoc is still unhappy, but poking around github there seems to be at least a handful of actively maintained projects using this code (though most would be better off using promhttp), so breakage isn't going to be free and this is a kinda-public API.

We can then decide if we want to merge that or keep working from a branch for the experiments.

I'd prefer not to have two APIs for this. To what extent are you planning on doing experiments, just local or getting it into a Prometheus release?

beorn7 · 2020-01-19T18:01:44Z

I'd prefer not to have two APIs for this. To what extent are you planning on doing experiments, just local or getting it into a Prometheus release?

I want this to instrument targets with exemplars. prometheus/prometheus doesn't need to be released with any of these changes, but if we do this without breaking changes, there is also no harm in doing so.

beorn7 · 2020-01-19T18:09:32Z

OK, PHAL.

Now we have a Closer, but Go stdlib style as an interface upgrade to not break anybody.

OM will only be an option for content negotiation with a dedicated function to negotiate. It is marked in the doc string as going away once the experiment is over.

brian-brazil · 2020-01-19T18:54:38Z

expfmt/encode.go

+}
+
+// NegotiateOpenMetrics works like Negotiate but includes FmtOpenMetrics as an
+// option for the result. Note that this function is experimental and might


is temporary and will disappear

expfmt/encode.go

brian-brazil · 2020-01-19T18:56:32Z

expfmt/encode.go

+// option for the result. Note that this function is experimental and might
+// disappear once FmtOpenMetrics is fully supported and as such may be
+// negotiated by the normal Negotiate function.
+func NegotiateOpenMetrics(h http.Header) Format {


Can we avoid putting OpenMetrics in the name? NegotiateWithCloser or something maybe.
As-is, you'd expect it to prefer/only-negotiate OM.

NegotiateWithCloser wouldn't really convey the meaning, either. It would arguably distract from the actual purpose of this function.

NegotiateIncludingOpenMetrics perhaps?

NegotiateIncludingOpenMetrics sounds good.

brian-brazil · 2020-01-19T20:22:14Z

expfmt/encode.go

 	}
 	return FmtText
 }

-// NewEncoder returns a new encoder based on content type negotiation.
+// NewEncoder returns a new encoder based on content type negotiation. The
+// caller is responsible to check if the returned Encoder also implements


We always implement Closer, we're only not returning it for backwards compatibility. I'd suggest saying to always close.

I changed the doc comment.

Signed-off-by: beorn7 <beorn@grafana.com>

brian-brazil · 2020-01-19T23:57:46Z

👍

beorn7 · 2020-01-20T00:28:47Z

Thanks. After all this careful consideration, I'll squash this into one commit as I don't think we will take those steps apart again.

beorn7 requested review from brian-brazil and cstyan January 18, 2020 23:03

brian-brazil reviewed Jan 18, 2020

View reviewed changes