client_golang icon indicating copy to clipboard operation
client_golang copied to clipboard

Fix float64 comparison test failure on archs using FMA

Open sbunce opened this issue 3 years ago • 8 comments

Background

Architectures using FMA optimization yield slightly different results so we cannot assume floating point values will be precisely the same across different architectures. https://github.com/prometheus/client_golang/pull/899#issuecomment-1244506390

Change

The solution in this change is to check "abs(a-b) < tolerance" instead of comparing the exact values. This will give us confidence that the histogram buckets are near identical.

Testing

I have not tested this change on one of the previously failing architectures. Help from someone with access to one of these architectures would be appreciated.

sbunce avatar Sep 13 '22 16:09 sbunce

There are better ways of a tolerant comparison. We already have https://github.com/prometheus/prometheus/blob/main/promql/test.go#L637 in the Prometheus code base. Perhaps that's easy enough to just copy over here.

beorn7 avatar Sep 20 '22 18:09 beorn7

Or depend on https://github.com/beorn7/floats :o)

beorn7 avatar Sep 20 '22 18:09 beorn7

Or depend on https://github.com/beorn7/floats :o)

I would prefer not to have to package yet another tiny dependency to make this build on Debian (and derivatives), so I would advocate just copying that function.

Or use something like https://pkg.go.dev/github.com/stretchr/testify/assert?utm_source=godoc#InDeltaSlice

dswarbrick avatar Sep 20 '22 19:09 dswarbrick

Yeah, the dependency was meant as a joke. I think the technique implemented in https://github.com/prometheus/prometheus/blob/main/promql/test.go#L637 (or in a slightly more general form in https://github.com/beorn7/floats ) works better that the approach in testify. And yes, I would propose to just copy that piece of code, maybe to the already existing prometheus/internal package.

beorn7 avatar Sep 27 '22 13:09 beorn7

Sounds like there's consensus on avoiding the extra dependency. Reminds me of that go proverb, "a little copying is better than a little dependency".

I copied the contents of https://github.com/beorn7/floats to the prometheus/internal package. I made a few small changes.

  • I renamed "ε" to "epsilon". I feel like this improves readability for non-math people slightly. I can switch back to the original variable name if people want.
  • I modified the comment to reference the original repo.
  • I added variants which operate on slices of floats.

Couple points I anticipate there may be disagreement about. Calling these out.

  • I haven't copied over the tests. Seems like vendoring doesn't usually grab the tests, but this is a slightly different situation.
  • I copied over the float32 variant of things, even though we don't use it. Let me know if I should remove those.

sbunce avatar Sep 27 '22 16:09 sbunce

I renamed "ε" to "epsilon".

I love the ε, but I am well aware that I'm a small minority here. (Insert lament about decline of classical education…)

beorn7 avatar Sep 28 '22 10:09 beorn7

I copied over the float32 variant of things.

I think it is overwhelmingly likely that we'll never use the float32 here. I would prefer to remove this variant. It's easy to add later should we really need it, thanks to the reference back to the source repo.

All other points you made seem fine to me.

Thanks for your contribution and your patience.

beorn7 avatar Sep 28 '22 10:09 beorn7

You're welcome. I appreciate your feedback.

  • Removed the float32 variants. I bet you're right that this will never be used.
  • Moved the function to a new "almost_equal.go" file. I didn't realize the difflib file was a fork of a vendored dependency. Makes sense to not mix those. More properly put the copyright from the original repo at the top of the new file.

sbunce avatar Sep 28 '22 15:09 sbunce