Eagerly verify Sample values are float-convertible

[mhansen]

Addresses #527.

There's a test setting GaugeHistogramMetricFamily.gsum_value = None, so I've preserved that behaviour, by not appending and crashing if gsum_value is None. That meant that this test case "([('spam', 0), ('eggs', 0)], None, TypeError)" doesn't throw any more, so I've deleted the test case.

I was a bit unsure about this bit: assert isinstance(exception.args[-1], core.Metric)

I'm not sure what exception.args[-1] is, the python docs for TypeError and ValueError don't explain. I've removed the assertion.

Not sure if removing these assertions is a great idea - would love feedback on this.

[brian-brazil]

I've preserved that behaviour, by not appending and crashing if gsum_value is None.

That's the right thing to do. gsum is technically optional.

[mhansen]

Thanks for the quick review. I pushed a new version, hope you don't mind me marking the comments as resolved, please reopen if any further questions on them.

[mhansen]

Could you elaborate? It seemed to me like whatever we pass to value, it’s converted to a float for output in floatToGoString. What constraints mean we have to keep ints ints?

On Fri, 20 Mar 2020 at 21:45, Brian Brazil [email protected] wrote:

@brian-brazil commented on this pull request.

In prometheus_client/samples.py https://github.com/prometheus/client_python/pull/530#discussion_r395556986 :

@@ -32,12 +32,17 @@ def gt(self, other):

Timestamp and exemplar are optional.

-# Value can be an int or a float. +# Value must be a float.

Actually this is a problem, we must support ints too. If someone passes in an int, it needs to stay as an int.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/prometheus/client_python/pull/530#pullrequestreview-378380027, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZYOJZUV2W4W35NE7PXKDRINCL5ANCNFSM4LPGANFQ .

[mhansen]

Oh, is it so you can +1 your counter and have that work even when you run out of floating point precision to represent a +1 step? Yeah that’s important

On Sat, 21 Mar 2020 at 08:06, Mark Hansen [email protected] wrote:

Could you elaborate? It seemed to me like whatever we pass to value, it’s converted to a float for output in floatToGoString. What constraints mean we have to keep ints ints?

On Fri, 20 Mar 2020 at 21:45, Brian Brazil [email protected] wrote:

@brian-brazil commented on this pull request.

In prometheus_client/samples.py https://github.com/prometheus/client_python/pull/530#discussion_r395556986 :

@@ -32,12 +32,17 @@ def gt(self, other):

Timestamp and exemplar are optional.

-# Value can be an int or a float. +# Value must be a float.

Actually this is a problem, we must support ints too. If someone passes in an int, it needs to stay as an int.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/prometheus/client_python/pull/530#pullrequestreview-378380027, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZYOJZUV2W4W35NE7PXKDRINCL5ANCNFSM4LPGANFQ .

[brian-brazil]

Yes, OpenMetrics has int support for that so we need it.

[mhansen]

FYI keeping ints as ints isn't the behaviour today: it looks like the current openmetrics exposition.py always outputs ints as floats today (uses floatToGoString), see this test that inputs 17 and outputs 17.0: https://github.com/prometheus/client_python/blob/master/tests/openmetrics/test_exposition.py#L54

I can change it to keep ints as ints, but could you spell out the requirement for me so I can add a test case for it?

• is it to output openmetrics "17" when input "17"?
• is it for avoiding precision issues with floats when you increment a large float and it rounds down to the same float?

On Sat, 21 Mar 2020 at 09:12, Brian Brazil [email protected] wrote:

Yes, OpenMetrics has int support for that so we need it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/prometheus/client_python/pull/530#issuecomment-601932257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZYOJHVUR75MOWD4WHC43RIPS53ANCNFSM4LPGANFQ .

[brian-brazil]

• is it to output openmetrics "17" when input "17"?

Yes

is it for avoiding precision issues with floats when you increment a large float and it rounds down to the same float?

This isn't something this client does currently, for direct instrumentation everything is a float.

[mhansen]

Had a go at preserving integer formatting in the openmetrics exposition.

[mhansen]

Hi, sorry for delay, this is still on my radar, but I haven't had a lot of time lately -- still intending to finish this.

On Thu, 21 May 2020 at 19:26, Brian Brazil [email protected] wrote:

@brian-brazil commented on this pull request.

Can you add a test for us catching this earlier now? If you rebase that CI failure should also go away.

In prometheus_client/samples.py https://github.com/prometheus/client_python/pull/530#discussion_r428543751 :

@@ -36,8 +36,17 @@ def gt(self, other):

Timestamp can be a float containing a unixtime in seconds,

a Timestamp object, or None.

Exemplar can be an Exemplar object, or None.

-Sample = namedtuple('Sample', ['name', 'labels', 'value', 'timestamp', 'exemplar']) -Sample.new.defaults = (None, None) +sample = namedtuple('Sample', ['name', 'labels', 'value', 'timestamp', 'exemplar']) + + +# Wrap the namedtuple to provide eager type-checking that value is a float.

Convertable to a float?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/prometheus/client_python/pull/530#pullrequestreview-416002579, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZYOMAALSLXQROOOHFTS3RSTXV7ANCNFSM4LPGANFQ .