Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the format strings compatible with Python 2.6 #361

Merged
merged 4 commits into from
Jan 4, 2019

Conversation

vgrigoriu
Copy link
Contributor

This should restore compatibility with Python 2.6, current version fails with:

Traceback (most recent call last):
...
  File "/usr/lib/python2.6/site-packages/prometheus_client-0.5.0-py2.6.egg/prometheus_client/exposition.py", line 194, in write_to_textfile
    f.write(generate_latest(registry))
  File "/usr/lib/python2.6/site-packages/prometheus_client-0.5.0-py2.6.egg/prometheus_client/exposition.py", line 119, in generate_latest
    output.append(sample_line(s))
  File "/usr/lib/python2.6/site-packages/prometheus_client-0.5.0-py2.6.egg/prometheus_client/exposition.py", line 86, in sample_line
    s.name, labelstr, floatToGoString(s.value), timestamp)
  File "/usr/lib/python2.6/site-packages/prometheus_client-0.5.0-py2.6.egg/prometheus_client/utils.py", line 21, in floatToGoString
    mantissa = '{}.{}{}'.format(s[0], s[1:dot], s[dot+1:]).rstrip('0.')
ValueError: zero length field name in format

@brian-brazil
Copy link
Contributor

Can you add the DCO?

Signed-off-by: Victor Grigoriu <vgrigoriu@gmail.com>
@vgrigoriu
Copy link
Contributor Author

Done.

@brian-brazil
Copy link
Contributor

The tests are failing on 2.6, are you sure this is broken?

@vgrigoriu
Copy link
Contributor Author

vgrigoriu commented Jan 3, 2019

Looking into the failing tests.

@vgrigoriu
Copy link
Contributor Author

So, I think I figured things out. The tests fail in Python 2.6 because of this line.

The test tests that all those values generate ValueErrors. This passes in Python 2.7 because floatToGoString('1.5555555555555201e+06') is '1.55555555555552e+06' -- see this line.

In Python 2.6, without my changes, the test passes because this line, coincidentally, throws ValueError (because 2.6 requires numbers for placeholders).

With my changes, this line no longer throws ValueError and it happens that floatToGoString(1.5555555555555201e+06) is '1.5555555555555201e+06' (the same), so the expected ValueError is not thrown.

The difference in the result of floatToGoString is that in 2.6 repr(1.5555555555555201e+06) is '1555555.5555555201' and in 2.7 it is '1555555.55555552'.

Obviously I don't know why this specific value was chosen for this test, but maybe we should remove it? What do you think @brian-brazil?

Signed-off-by: Victor Grigoriu <vgrigoriu@gmail.com>
@brian-brazil
Copy link
Contributor

Odd that CI let this though.

Obviously I don't know why this specific value was chosen for this test, but maybe we should remove it?

It's there specifically to catch this. This sounds like https://bugs.python.org/issue7117 or https://bugs.python.org/issue7632

Signed-off-by: Victor Grigoriu <vgrigoriu@gmail.com>
@vgrigoriu
Copy link
Contributor Author

Odd that CI let this though.

This passed in CI for 2.6 because the only test path exercising this code happens to be in a test that expects ValueError.

I pushed a change that makes floatToGoString work the same for the existing test cases in 2.6 and 2.7.

@@ -18,6 +18,6 @@ def floatToGoString(d):
# Go switches to exponents sooner than Python.
# We only need to care about positive values for le/quantile.
if d > 0 and dot > 6:
mantissa = '{0}.{1}{2}'.format(s[0], s[1:dot], s[dot+1:]).rstrip('0.')
mantissa = '{0}.{1}{2}'.format(s[0], s[1:dot], s[dot+1:16]).rstrip('0.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is correct, as it's the underlying algorithm that's not right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that there's no easy way to preserve compatibility with Python 2.6?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly not, we'd need our own floating point renderer for it.

May I ask why you're still on 2.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my sins :-P

I'll close the PR then, I'm good with using 0.4.2 until we can upgrade to Python 2.7.

@vgrigoriu vgrigoriu closed this Jan 4, 2019
@vgrigoriu vgrigoriu deleted the py_26_compat branch January 4, 2019 13:50
@brian-brazil
Copy link
Contributor

We should still fix the format bug on 2.6

@vgrigoriu vgrigoriu restored the py_26_compat branch January 4, 2019 14:03
@vgrigoriu
Copy link
Contributor Author

So what do you want to do about the test failing on 2.6?

@vgrigoriu vgrigoriu reopened this Jan 4, 2019
@bz2
Copy link
Contributor

bz2 commented Jan 4, 2019

Can skip the test on 2.6 only with a message referencing the floating point formatting compatibility issue.

Signed-off-by: Victor Grigoriu <vgrigoriu@gmail.com>
@brian-brazil brian-brazil merged commit 2230511 into prometheus:master Jan 4, 2019
@brian-brazil
Copy link
Contributor

Thanks!

@vgrigoriu vgrigoriu deleted the py_26_compat branch January 4, 2019 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants