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

Update label validation to run once per metric #369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jdhudson3
Copy link

@jdhudson3 jdhudson3 commented Apr 4, 2022

This is a minor performance improvement during the label parsing stages of metric handling.

When running a profiler on an application that utilizes this library, we noticed that this validation was 1/3 of the time spent in startLabelName and was an easy minor performance win.

When parsing metric files with more significant numbers of tags, this validation scales poorly and leads to a significant number of maps being created/iterated through. This may cause a slight decrease in performance when parsing invalidly tagged metrics in some cases, but will increase performance on the happy path. This performance result is not noticeably faster in the performance test below due to to a low number of tags used in the unit tests.

Bench results:
This branch:

pkg: github.com/prometheus/common/expfmt
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkTextParse
BenchmarkTextParse-16     	    6176	    190301 ns/op
BenchmarkParseError
BenchmarkParseError-16    	   23068	     53793 ns/op
PASS

Main:

pkg: github.com/prometheus/common/expfmt
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkTextParse
BenchmarkTextParse-16     	    5448	    193167 ns/op
BenchmarkParseError
BenchmarkParseError-16    	   22438	     54203 ns/op
PASS

When tested in isolation a more clear performance improvement is shown. The happy path test is tested with with 25 metrics, each containing 8 tags. The error case is tested with one metric with 9 tags, the last being a duplicate of the first.
This branch:

pkg: github.com/prometheus/common/expfmt
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkTextParse
BenchmarkTextParse-16     	    9082	    113003 ns/op
BenchmarkParseError
BenchmarkParseError-16    	  294361	      4102 ns/op
PASS

Main:

pkg: github.com/prometheus/common/expfmt
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkTextParse
BenchmarkTextParse-16     	    7752	    140930 ns/op
BenchmarkParseError
BenchmarkParseError-16    	  253125	      5157 ns/op
PASS

Tagging for review: @roidelapluie

Signed-off-by: jdhudson3 43144225+jdhudson3@users.noreply.github.com

Signed-off-by: jdhudson3 <43144225+jdhudson3@users.noreply.github.com>
@jdhudson3 jdhudson3 changed the title Update label validation Update label validation to run once per metric Apr 4, 2022
@roidelapluie
Copy link
Member

Thanks! I think the current path is more 'logical' and easy to use than the proposal. Does this really cause performance issues?

@jdhudson3
Copy link
Author

I can see what you mean that the initial approach fits in the code a little more obviously, I do think though that this fits the overall flow of the state machine as well.

I can't speak for anyone else's use of the package, but in our case we are parsing a significant number of metrics that each contain a minimum of 8 tags. This at minimum creates/destroys 7 extra hashmaps with 28 extra assignments per metric and scales as O(n^2).

In our case this is significant enough to care about, as it represents an 8% overall increase on our applications performance.

Attached below is a screenshot of our profiler, currently map specific operations take up 38% of the time to parse a metric label. This proposal should cut it to ~8%.

Screen Shot 2022-04-15 at 9 06 11 AM

@roidelapluie
Copy link
Member

Thanks, makes sense. I will take a deeper look.

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.

None yet

2 participants