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

feat: improve performance of getMetricAsPrometheusString #542

Merged
merged 4 commits into from
Mar 4, 2023
Merged

feat: improve performance of getMetricAsPrometheusString #542

merged 4 commits into from
Mar 4, 2023

Conversation

shappir
Copy link
Contributor

@shappir shappir commented Feb 26, 2023

Use Array.prototype.join instead string concatenation for constructing Prometheus metric string. This results in improved performance and reduced memory consumption, especially when there's a large amount of dimensions (labels and label values). In addition uses array iteration methods instead of explicit loops.

@SimenB
Copy link
Collaborator

SimenB commented Feb 27, 2023

Thanks! Do you have any benchmarks before/after?

https://github.com/siimon/prom-client/tree/master/benchmarks

@shappir
Copy link
Contributor Author

shappir commented Mar 1, 2023

This change primarily impacts the performance of metrics that have a large number of rows in the Prometheus metrics output, e.g. histograms with several labels. In our application I saw metric generation time reduced by ~50%. This is not surprising since the number of times string values are copied as result of concatination is the same order as the number of rows.

In the benchmark test I got 50% faster and 50% "acceptably slower". Every once in a while I would get one entry or another as slower, but it was inconsistent.

@SimenB
Copy link
Collaborator

SimenB commented Mar 4, 2023

Cool, thanks!

@SimenB SimenB merged commit cf5173c into siimon:master Mar 4, 2023
@SimenB
Copy link
Collaborator

SimenB commented Mar 6, 2023

Hey @shappir! I just released this in 14.2.0.

However, I'm having some issues trying to rebase next on master on top of #544. You wouldn't by any chance be able to send a forward-porting PR to next applying the changes in this PR?

@shappir
Copy link
Contributor Author

shappir commented Mar 7, 2023

Is this still an issue? Do you need me to do anything?

@SimenB
Copy link
Collaborator

SimenB commented Mar 7, 2023

If it's not too much trouble, a PR against next applying the same optimizations would be very helpful 🙂

@shappir
Copy link
Contributor Author

shappir commented Mar 8, 2023

Created a pr for next: #548

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.

2 participants