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.

[brian-brazil]

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

[mhansen]

Had a go at this, split over two commits, what do you think?

[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 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In prometheus_client/samples.py <#530 (comment)> : > @@ -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 <#530 (review)>, 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 ***@***.***> 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 ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In prometheus_client/samples.py > <#530 (comment)> > : > > > @@ -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 > <#530 (review)>, > 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 ***@***.***> 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 <#530 (comment)>, 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.

[brian-brazil]

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

[brian-brazil]

Convertable to a float?

[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 ***@***.***> wrote: ***@***.**** 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 <#530 (comment)> : > @@ -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 <#530 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZYOMAALSLXQROOOHFTS3RSTXV7ANCNFSM4LPGANFQ> .