Float metrics values overflow on ARM systems

[bekriebel]

When compiled for ARM, metrics values are overflowing when they are cast from float64 to int. This seems to be caused by ARM using int32 instead of int64 during the conversion. This results in high-value metrics getting set to either 2147483.647 or 0 (depending on which GOARM value is used) as soon as the value overflows int32.

The code causing this:

cloudprober/metrics/float.go

Lines 92 to 95 in 882d33f

	// Truncate the float to 3-digit precision.
	// Example: 3.33333333333 -> 3.333
	ff := float64(int(f.f*1000)) / 1000
	return strconv.FormatFloat(ff, 'f', -1, 64)

This can be fixed in a few ways:

1. Don't truncate at all and just output the float as-is.
2. Specify int64 instead of int. However, this still has a chance of overflow for large values.
3. Instead of using the int cast and str.FormatFloat, use fmt.Sprintf("%.3f", f.f). This performs the truncate but does not remove the trailing zeros in the decimal in the same way FormatFloat currently does.
4. Use fmt.Sprintf, but also perform strings.TrimRight to remove trailing zeros and the decimal point where valid. This is functionally similar to the existing version.

I implemented the fourth option on an incoming pull request. Let me know if you'd prefer a different version of the fix.

[manugarg]

@bekriebel This is certainly a bug. Thanks for debugging this and the PR. Cloudprober's source of truth (SoT) is Google internal, which means that I'll have to import his pull request to the internal source and then export it back. Thankfully, this process preserves the original author.

Unfortunately, cloudprober was not setup properly for this kind of transformations (reverse). I was trying to sort that out over past few days. That's why delay in accepting the PR. I'll try to finish this process tomorrow.

[bekriebel]

No problem. I already have it running in my environment with this change and it is working well. This seems to be the only change necessary for the code to work well on the ARM platform.

Thanks for your work on it!

[manugarg]

@bekriebel Thanks. I wanted to ask if you'd mind if I get rid of the trimming completely (that is, we'll print trailing zeros) . I think it's making code a bit obscure and not adding as much value.

I can just do it at my end if that sounds good to you.

[bekriebel]

@manugarg Yeah, that was my first inclination as well but wasn't sure if it was done this way for a reason. What do you think about just returning fmt.Sprintf("%f", f.f)? Since this is most likely being ingested by a receiver that is accepting it as a float anyway, I don't see the need to truncate the string at all. If a receiver can't accept this, it could always be truncated in the specific surfacer.

[bekriebel]

I went ahead and pushed a new version that doesn't trim or truncate. Personally, I prefer having the extra precision. It can always be truncated on a UI side or in a surfacer if desired.

[manugarg]

Importing this change back as #119. Please take a look.

Keeping precision of 3 decimal points as sometimes we get really long strings in the latency (.3333333.. kind of things). Also, using strconv instead of fmt.Sprintf as strconv is a bit faster.

[manugarg]

Thanks again, Brint. Closing this issue now.