Text parser optimization (~4.5x perf)

[mfpierre]

Hi,

As we extensively use this library for parsing prometheus text format, we have observed that for big payloads (like https://github.com/kubernetes/kube-state-metrics for a big cluster) that can contains 400k+ lines the text parsing can be quite slow. (up to 27secs)

We tried to optimize the parser by dropping the state machine and leveraging native python functions such as index or find and it gives us an average of x5 performances.

Here are some benchmark using timeit:

call (x100000): _parse_sample('simple_metric 1.513767429e+09')
previous implementation: 1.10845804214
new implementation: 0.277444839478
improvement: x3.99523755508

call (x100000): _parse_sample('kube_service_labels{label_app="kube-state-metrics",label_chart="kube-state-metrics-0.5.0",label_heritage="Tiller",label_release="ungaged-panther",namespace="default",service="ungaged-panther-kube-state-metrics"} 1')
previous implementation: 7.58089208603
new implementation: 1.48280119896
improvement: x5.11254785291

For the KSM payload (400k lines) the parsing goes from ~27sec to ~4.7sec

Note: We could go up to almost 10x performance if we dropped some edge-cases treatment (like escaping, tab/space, etc...) could we consider a "strict" parsing mode that we could optionally use for "good citizens"?

[brian-brazil]

could we consider a "strict" parsing mode that we could optionally use for "good citizens"?

Those aspects of the text format are not optional, they must be implemented to have a correct parser.

[brian-brazil]

Performance improvements would be great, however the format has many ways it can be represented and this code only parses a subset of the potential valid input. If you can manage to make it work with that, that'd be great.

[brian-brazil]

A __str__ would make more sense I think

[mfpierre]

renamed it to __str__ and having repr call str because it's useful for comparing unit test output:

E       First differing element 0:
E       <Metric name: a, documentation: help, type: counter, samples: [(u'a', {u'foo': u'bar'}, 1), (u'a', {u'foo': u'baez'}, 2), (u'a', {u'foo': u'buz'}, 3)]>
E       <Metric name: a, documentation: help, type: counter, samples: [(u'a', {u'foo': u'bar'}, 1.0), (u'a', {u'foo': u'baz'}, 2.0), (u'a', {u'foo': u'buz'}, 3.0)]>
E
E       - [<Metric name: a, documentation: help, type: counter, samples: [(u'a', {u'foo': u'bar'}, 1), (u'a', {u'foo': u'baez'}, 2), (u'a', {u'foo': u'buz'}, 3)]>]
E       ?                                                                                                                  -
E
E       + [<Metric name: a, documentation: help, type: counter, samples: [(u'a', {u'foo': u'bar'}, 1.0), (u'a', {u'foo': u'baz'}, 2.0), (u'a', {u'foo': u'buz'}, 3.0)]>]
E       ?

instead of

E       First differing element 0:
E       <prometheus_client.core.CounterMetricFamily object at 0x108018c50>
E       <prometheus_client.core.Metric object at 0x108018cd0>
E
E       - [<prometheus_client.core.CounterMetricFamily object at 0x108018c50>]
E       ?                          -------      ------                    ^
E
E       + [<prometheus_client.core.Metric object at 0x108018cd0>]

[brian-brazil]

Usually str calls repr rather than the other way around. repr should usually be an instantiatable version of object, while str is more human readable.

[mfpierre]

Modified repr to fit what it's supposed to be, still way more readable for tests outputs 👍

[brian-brazil]

Help and label values have different escaping rules (double quote is the difference), you need two functions for this.

[mfpierre]

added another function specific for Help

[brian-brazil]

Please keep comments as full sentances

[brian-brazil]

Any mix of any number of spaces and tabs is permitted

[mfpierre]

Added some additional unit tests around this 👍

[brian-brazil]

there can be whitespace after the brace, and basically everywhere else between things

[mfpierre]

this should be covered, I added one additional test case in test_spaces

[brian-brazil]

A label value could contain a }

[mfpierre]

This already taken into account with the use of rindex, added a test case to validate this point.

[brian-brazil]

There can be a trailing comma after the last "

[mfpierre]

should be already covered, test_commas is validating this

[brian-brazil]

What if if you have x="" as the label? i - 1 will be -1, which might have unexpected results

[mfpierre]

added a unit-test around empty labels, but this works fine 👍

[brian-brazil]

Thanks!

[mfpierre]

Hey @brian-brazil, any plans to do a release soon? 🙇

[brian-brazil]

I've added it to my todo list