Skip to content

HDDS-13014. Improve PrometheusMetricsSink#normalizeName performance #8438

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

Merged
merged 3 commits into from
May 22, 2025

Conversation

ivandika3
Copy link
Contributor

@ivandika3 ivandika3 commented May 13, 2025

What changes were proposed in this pull request?

From the 5 minutes flamegraph of S3G, we see that nearly 40% of the CPU usage are spent on PrometheusMetrics#putMetrics.
Moreover, 30% of the CPU usage are attributed only for PrometheusMetricsSinkUtil#normalizeName. We see that most of the time are spent on regex matching (probably due to the lookbehind and the lookahead mechanisms).

We need to find a way to improve the performance
Two possible improvements

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13014

How was this patch tested?

Clean CI: https://github.com/ivandika3/ozone/actions/runs/15001669302

Simple microbenchmark. See: https://issues.apache.org/jira/secure/attachment/13076478/TestPrometheusMetricsSinkUtilPerformance.java

# With cache

Warming up...

Running performance test...

Performance Test Results:
Total test cases: 5
Total iterations: 100000
Total operations: 500000
Total time: 47.00 ms
Average time per operation: 0.000 ms

Testing cache hit performance...
Cache hit results:
Total time with cache: 39.00 ms
Average time per operation with cache: 0.000 ms


# Without cache

Warming up...

Running performance test...

Performance Test Results:
Total test cases: 5
Total iterations: 100000
Total operations: 500000
Total time: 1236.00 ms
Average time per operation: 0.002 ms

Testing cache hit performance...
Cache hit results:
Total time with cache: 1167.00 ms
Average time per operation with cache: 0.002 ms

@ivandika3 ivandika3 marked this pull request as ready for review May 14, 2025 01:16
@ivandika3
Copy link
Contributor Author

ivandika3 commented May 14, 2025

Currently for datanode, if datanode still uses the schema V2 (i.e. one RocksDB per container), there might be high memory overhead due to the RocksDB metrics.

I think to reduce the number of metrics stored in the cache, we might need to change the metrics to use tag instead
So instead of

rocksdb_ds_4545195c_120e_47c0_832a_25c4e618250e_container_db_write_raw_block_micros_percentile95{hostname="<redacted>"}

We use

rocksdb_container_db_write_raw_block_micros_percentile95{storageid="ds_4545195c_120e_47c0_832a_25c4e618250", hostname="<redacted>"}

This applies to other similar metrics

@adoroszlai
Copy link
Contributor

Thanks @ivandika3 for the patch. Can we use some kind of size-limited cache?

we might need to change the metrics to use tag
instead of rocksdb_ds_4545195c_120e_47c0_832a_25c4e618250e_container_db_write_raw_block_micros_percentile95{hostname="<redacted>"}
We use rocksdb_container_db_write_raw_block_micros_percentile95{storageid="ds_4545195c_120e_47c0_832a_25c4e618250", hostname="<redacted>"}

This applies to other similar metrics

I think that would also improve usability of metrics, regardless of the cache size problem.

@ivandika3
Copy link
Contributor Author

@adoroszlai Thanks for the review. I have updated the patch to use a fixed cache size with max size 100,000. Please let me know what you think.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the patch to use a fixed cache size with max size 100,000

Thanks @ivandika3 for updating the patch, LGTM.

// Original metric name -> Normalized Prometheus metric name
private static final CacheLoader<String, String> NORMALIZED_NAME_CACHE_LOADER =
CacheLoader.from(PrometheusMetricsSinkUtil::normalizeImpl);
private static final com.google.common.cache.LoadingCache<String, String> NORMALIZED_NAME_CACHE =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (change only if you need to update the patch for any other reason): add import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, previously I was using Guava Cache interface that conflicts with our internal Cache class.

@adoroszlai adoroszlai requested a review from kerneltime May 16, 2025 10:10
@ivandika3
Copy link
Contributor Author

ivandika3 commented May 20, 2025

FYI, raised a similar patch to Hadoop as HADOOP-19571 (apache/hadoop#7692)

@adoroszlai adoroszlai merged commit 87dfa5a into apache:master May 22, 2025
42 checks passed
@adoroszlai
Copy link
Contributor

Thanks @ivandika3 for the patch.

@ivandika3 ivandika3 deleted the HDDS-13014 branch May 22, 2025 14:20
@ivandika3
Copy link
Contributor Author

Thanks @adoroszlai for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants