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

optimize Collector.sanitizeMetricName #777

Merged
merged 1 commit into from
May 2, 2022

Conversation

fwbrasil
Copy link
Contributor

Problem

Some libraries resolve Prometheus metrics dynamically and use Collector.sanitizeMetricName for that. Ideally, this method shouldn't be in the hot path but it seems worth optimizing it for libraries that provide dynamic metric resolution like https://github.com/clj-commons/iapetos so the performance penalty isn't a concern for them.

I noticed this issue while profiling an application that uses iapetos. The regex execution is currently the main cost of the application:

image

image

Including being the source of a large portion of the invoke interface dispatches:

image

Solution

Optimize the Collector.sanitizeMetricName method to avoid the regex engine and use a simple for-loop.

Results

I've created a new benchmark for this method. Results:

Before

Benchmark                                             Mode  Cnt    Score    Error  Units
SanitizeMetricNameBenchmark.sanitizeNonSanitizedName  avgt    4  694.406 ± 44.971  ns/op
SanitizeMetricNameBenchmark.sanitizeSanitizedName     avgt    4  486.219 ± 25.100  ns/op

After

Benchmark                                             Mode  Cnt   Score   Error  Units
SanitizeMetricNameBenchmark.sanitizeNonSanitizedName  avgt    4  17.915 ± 0.940  ns/op
SanitizeMetricNameBenchmark.sanitizeSanitizedName     avgt    4  13.730 ± 1.139  ns/op

Signed-off-by: Flavio Brasil <fwbrasil@gmail.com>
@fwbrasil fwbrasil force-pushed the optimize-sanitize-metric-name branch from 984d75a to 7f0f7a3 Compare April 26, 2022 21:26
@fwbrasil
Copy link
Contributor Author

Hi @fstab, I'm sorry to ping you directly. If you have an opportunity to take a look at this PR, we wanted to know if it's something that seems to make sense for you (it's ok if you can't review/merge atm). If not, we're going to try to optimize it in iapetos. Thank you!

@fstab
Copy link
Member

fstab commented Apr 29, 2022

Hi, thanks for the PR. I am planning to have a look this weekend, didn't find the time during the week.

@fstab fstab merged commit 989cbd1 into prometheus:master May 2, 2022
@fstab
Copy link
Member

fstab commented May 2, 2022

Thanks a lot @fwbrasil. I merged it.

shoothzj added a commit to apache/pulsar that referenced this pull request Jul 14, 2022
### Motivation
prometheus client 0.16.0 contains some approvements that we can benefit from. Thanks for @dave2wave @michaeljmarshall  the reminder and pointing out.

> [ENHANCEMENT] Reduce the number of core threads in HTTPServer from 5 to 1. The HTTPServer will still start up to 5 threads on demand if there are parallel requests, but it will use only 1 thread as long as requests are sequential (prometheus/client_java#786).
[ENHANCEMENT] Optimize metric name sanitization: Replace the regular expression with a hard-coded optimized algorithm to improve performance (prometheus/client_java#777). Thanks @fwbrasil

See https://github.com/prometheus/client_java/releases

### Modifications

Bump prometheus client version from 0.15.0 to 0.16.0

### Documentation

Check the box below or label this PR directly.

Need to update docs? 
  
- [x] `doc-not-needed` 
dependency updates, no need doc
Jason918 pushed a commit to apache/pulsar that referenced this pull request Sep 2, 2022
### Motivation
prometheus client 0.16.0 contains some approvements that we can benefit from. Thanks for @dave2wave @michaeljmarshall  the reminder and pointing out.

> [ENHANCEMENT] Reduce the number of core threads in HTTPServer from 5 to 1. The HTTPServer will still start up to 5 threads on demand if there are parallel requests, but it will use only 1 thread as long as requests are sequential (prometheus/client_java#786).
[ENHANCEMENT] Optimize metric name sanitization: Replace the regular expression with a hard-coded optimized algorithm to improve performance (prometheus/client_java#777). Thanks @fwbrasil

See https://github.com/prometheus/client_java/releases

### Modifications

Bump prometheus client version from 0.15.0 to 0.16.0

### Documentation

Check the box below or label this PR directly.

Need to update docs?

- [x] `doc-not-needed`
dependency updates, no need doc

(cherry picked from commit 948000b)
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