Bump client_model

[ArthurSens]

Opening this PR to make the review of #1313 easier.

This one is only about bumping the version of client_model in go.mod. This version upgrade is particularly difficult because, in client_model, it updates the version of protoc which changes how proto messages are cast into strings:

1. '<' and '>' characters were replaced with '{' and '}' respectively.
2. The text format is non-deterministic. More information in encoding: provide canonical output format golang/protobuf#1121
3. proto3 adds a new field to proto.Message(sizeCache) that breaks reflect.DeepEqual. Forcing us to use proto.Equal now.

[ArthurSens]

The only test I still couldn't figure out is TestHandler in prometheus/register_test.go, which compares multiline proto messages. ANy help appreciated 😅

[bwplotka]

Thanks!

Can we double check that:

• The newest protoc version has the { and that's the decision for future? Perhaps someone used old protoc and we will have to switch back ;p
• Are we sure this does not introduce random char discussed in Update Go modules #1270 (comment)? Since tests are mostly passing, I assume this does not.

What's the problem with multiline? If you need help could give us more details on why it's tricky?

[beorn7]

I'm pretty sure this will run into the test flakiness problem because the new protobuf code intentionally randomizes the text output.

[ArthurSens]

Ok, as discussed in slack, I started to refactor all tests that were using proto-text for comparison. Now they are using proto.Equal.

There are a few situations that I'm not sure how to solve yet though.

A few tests are Examples. Those examples compare what is being printed to what magic comment // Output:. For example:

func ExampleMetricVec() {
	infoVec := NewInfoVec(
		"library_version_info",
		"Versions of the libraries used in this binary.",
		[]string{"library", "version"},
	)

	infoVec.WithLabelValues("prometheus/client_golang", "1.7.1")
	infoVec.WithLabelValues("k8s.io/client-go", "0.18.8")

	// Just for demonstration, let's check the state of the InfoVec by
	// registering it with a custom registry and then let it collect the
	// metrics.
	reg := prometheus.NewRegistry()
	reg.MustRegister(infoVec)

	metricFamilies, err := reg.Gather()
	if err != nil || len(metricFamilies) != 1 {
		panic("unexpected behavior of custom test registry")
	}
	fmt.Println(metricFamilies[0].String())

	// Output:
	// name:"library_version_info" help:"Versions of the libraries used in this binary." type:GAUGE metric:<label:<name:"library" value:"k8s.io/client-go" > label:<name:"version" value:"0.18.8" > gauge:<value:1 > > metric:<label:<name:"library" value:"prometheus/client_golang" > label:<name:"version" value:"1.7.1" > gauge:<value:1 > >
}

I'm not familiar with tests functions called Example.*... What can we do here?

[ArthurSens]

Also, testHandler in prometheus/registry_test.go is such an extensive test. I'm having a hard time understanding how to fix that one 😓

[bwplotka]

This is a very common example on why random character decision is damaging.

I would propose creating "derandomizeProtoString" function that removes all instances of double whitespace into one (: Then I would use it only where it's absolutely necessary e.g. explictly when printing proto string.

[beorn7]

Maybe we could also convert protobuf to the Prometheus text format? In the spirit of github.com/prometheus/client_golang/prometheus/testutil .

[SuperQ]

For the error content in the Prometheus text output format I would suggest switching to protojson formatter.

[ArthurSens]

For the error content in the Prometheus text output format I would suggest switching to protojson formatter.

Unfortunately protojson adds the same brittle 😭

[bwplotka]

Nice, epic work!

Thanks for caring about bigger problem here.

I have suggestion for that special function type safety, readability etc. Once that is addressed - LGTM! Thanks!

[bwplotka]

Not sure where you got this "brittle" word from. (: Let's be specifc and call it "normalization" rather?

Also printing errors is odd if nothing guards callers from using this function for other things than printing.

Finally, we have opportunity to reduce boilerplate of marshalling too and make this function generally type safe. We know we want to use it only for printing in example, let's block other use cases, so further devs will be forced to produce golden tests instead (:

I would propose:

Suggested change

	func removeBrittleJSON(j []byte) string {
	func printlnNormalized(t testing.TB, msg proto.Message) {
	t.Helper()
	// marshall, do t.Fatal on error
	// compact json
	// print to stdout (e.g. fmt.Println)
	}

WDYT?

[ArthurSens]

Not sure where you got this "brittle" word from. (: Let's be specifc and call it "normalization" rather?

I guess I'm not 100% fluent in English and sometimes some words just get stuck in my head 😅. Agreed that normalization sounds better

Also printing errors is odd if nothing guards callers from using this function for other things than printing.

Finally, we have opportunity to reduce boilerplate of marshalling too and make this function generally type safe. We know we want to use it only for printing in example, let's block other use cases, so further devs will be forced to produce golden tests instead (:

I would propose:

WDYT?

Sounds good! Thanks for the suggestions

[ArthurSens]

Actually, examples don't have parameter testing.T, so calling 'printlnNormalized(t testing.TB...' doesn't seem possible?

I'm panic'ing instead of calling t.Fatal, but let me know if I missed something here

[bwplotka]

👍🏽 Good point, panic is fine

[bwplotka]

Thanks! Huge amount of work 💪🏽

LGTM