fix(parsers.prometheus): Histogram infinity bucket not being generated when using protobuf protocol

[mmolnar]

In some cases the infinity bucked is not being parsed or generated from text prometheus input by the library github.com/prometheus/common/expfmt

I have not found the specifics, but it must be related to the version of libc because the same telegraf binary works on ubuntu with libc6:amd64 2.27-3ubuntu1.6, but does not work on debian with 2.28-10.

Because the infinity bucket has the same value as histogram sample count we can just enforce creation of this bucket if it is missing. They are doing basically the same thing in the underlying library used to parse metrics when generating text format from metrics.

resolves #11490

[srebhan]

The code looks good to me. Thanks for the fix @mmolnar!

If you can create an issue that describes the issue in more detail and link it here that would be super awesome.

[srebhan]

Oh and maybe a test case to make sure we don't fail in the future?!

[mmolnar]

If you can create an issue that describes the issue in more detail and link it here that would be super awesome.

There is not much more to say about this because I did not find the root cause, this is just an work-around for the varied behaviour of the underlying library prometheus/common/expfmt

and maybe a test case to make sure we don't fail in the future?!

I am not sure how would I do that because it seems the whole issue is environment dependent.
I have tried the exact same test case scenario on 2 different linux distributions (ubuntu vs debian) and one worked properly and one did not generate metric with the Infinity bucket.

Might have something to do with the fact that value of the "le" label is actually +Infinity value of golang type float64, sometimes it gets interpreted as NoN and dropped and sometimes it gets converted to string "+Inf".
The test case for presence of +Inf in histogram is already there, so it seems you just did not encounter this issue in your build environment.

[mmolnar]

I have tried to look deeper into the issue and found the core of the problem. Issue is NOT environment dependent, but depends on the protocol being used.

Some prometheus clients are capable of responding with protobuf protcol which telegraf requests by default. My test cases were different, on ubuntu i tried to simulate the response with nginx which was capable to respond only in text expostion format, but on debian I had a service responding in protobuf.

Seems the prometheus protobuf exposition format expects Inf bucket to be optional, but it is not optional in the text exposition format, my pull request will fix this.

There could be an test case done for this, here I have 2 questions:

1. Is it necesary seeing as protobuf protocol is deprecated by Prometheus? Should telegraf just stop requesting and parsing protobuf protocol?
2. To test it we need an protobuf message to parse, should this be an base64 encoded binary data string in the test file? Or should the test file create the message for the parser? This could probably done from JSON to Go Data Model and then to binary.

[srebhan]

@mmolnar I would just test if the makeBuckets() function always returns the inf bucket, if given or not given, I don't think you need to do the whole loop.

Regarding the protobuf thingy, I'll bring up a discussion internally...

Anyway, it would be nice to copy this (maybe even as-is) into an issue so people that search for it will easily find it and make their way to this fix...

[telegraf-tiger]

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

👍 This pull request doesn't change the Telegraf binary size

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

[mmolnar]

@srebhan I created an issue #11490 with the relevant findings and created an test case for this problem.

[srebhan]

Hey @mmolnar, sorry for the late reply! We discussed deprecating on how to handle this issue in an internal meeting and we think deprecating the protobuf part should be done in 2.0 as this is a potential breaking change. So the target setting is to get this fix in and deprecate the root cause later. Does that sound good?

[srebhan]

Looks good to me. Thanks a bunch @mmolnar for hunting this down and submitting this fix!

[mmolnar]

So the target setting is to get this fix in and deprecate the root cause later. Does that sound good?

Yes, thank you.