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

Use bulk string writes for text formats #1273

Merged
merged 3 commits into from
Feb 18, 2025

Conversation

marschall
Copy link
Contributor

Further optimize TextFormatUtil#writeEscapedLabelValue. We're seeing TextFormatUtil#writeEscapedLabelValue show up in our production traces due to the single char writes to OutputStreamWriter. We're using OpenMetricsTextFormatWriter. #1241 and #1248 should take care of most of these issues but there still remains some optimization potential left. BufferedWriter#write(int) has some minimal overhead in terms of locking. If we assume that rarely any characters need to be escaped and instead optimize to write as large of a part of the String as possible in one method call.

Before

Benchmark                                             Mode  Cnt       Score      Error  Units
TextFormatUtilBenchmark.openMetricsWriteToByteArray  thrpt   25  438485.372 ± 4270.355  ops/s
TextFormatUtilBenchmark.openMetricsWriteToNull       thrpt   25  440105.281 ± 2891.572  ops/s
TextFormatUtilBenchmark.prometheusWriteToByteArray   thrpt   25  467213.001 ±  878.780  ops/s
TextFormatUtilBenchmark.prometheusWriteToNull        thrpt   25  472931.759 ±  976.028  ops/s

After

Benchmark                                             Mode  Cnt       Score      Error  Units
TextFormatUtilBenchmark.openMetricsWriteToByteArray  thrpt   25  462852.243 ± 5071.696  ops/s
TextFormatUtilBenchmark.openMetricsWriteToNull       thrpt   25  469910.681 ± 1670.430  ops/s
TextFormatUtilBenchmark.prometheusWriteToByteArray   thrpt   25  482362.506 ± 2051.684  ops/s
TextFormatUtilBenchmark.prometheusWriteToNull        thrpt   25  487707.557 ± 3344.881  ops/s

About a 5% to 6% gain for OpenMetricsTextFormatWriter and around 3% for PrometheusTextFormatWriter.

Note that this benchmark is actually for OpenMetricsTextFormatWriter and PrometheusTextFormatWriter since TextFormatUtil is not public and can therefore not be benchmarked directly. The relative gains in #writeEscapedLabelValue are higher because the benchmark runs the full text format writers.

I also added a test for TextFormatUtil#writeEscapedLabelValue since the code is now more complicated.

Signed-off-by: Philippe Marschall <[email protected]>
Optimize TextFormatUtil to prefer bulk writes.

Signed-off-by: Philippe Marschall <[email protected]>
@zeitlinger
Copy link
Member

looks good - thanks for the contribution 😄

@marschall
Copy link
Contributor Author

The build fails because of the lack of test coverage in prometheus-metrics-exposition-textformats. Should I increase the test coverage or lower the exception in pom.xml?

@zeitlinger
Copy link
Member

The build fails because of the lack of test coverage in prometheus-metrics-exposition-textformats. Should I increase the test coverage or lower the exception in pom.xml?

I merged a PR to increase the coverage

@dhoard dhoard merged commit 7016bf7 into prometheus:main Feb 18, 2025
4 checks passed
@marschall
Copy link
Contributor Author

Awesome, thanks for the quick merge.

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