[native] Update prometheus metrics asynchronously #24716

jaystarshot · 2025-03-12T18:05:35Z

Description

Make metric updation async to rule out any slowness in the main thread. This is to rule out any slowness due to extensive metric use as we had seen such issue for histograms beo

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

jaystarshot · 2025-03-17T16:30:16Z

@aditi-pandit @majetideepak Can you please help review? Thanks

majetideepak

@jaystarshot Can you share some data on the evaluation of this change? What are the benefits and what are the cons?

presto-native-execution/presto_cpp/main/runtime-metrics/tests/PrometheusReporterTest.cpp

majetideepak · 2025-03-17T20:56:22Z

presto-native-execution/presto_cpp/main/runtime-metrics/tests/PrometheusReporterTest.cpp

@@ -20,7 +20,7 @@ namespace facebook::presto::prometheus {
 class PrometheusReporterTest : public testing::Test {
 public:
  void SetUp() override {
-    reporter = std::make_shared<PrometheusStatsReporter>(testLabels);
+    reporter = std::make_shared<PrometheusStatsReporter>(testLabels, 1);


Why not test with multiple threads?

The problem is that for gauge metric its the last value which stays so the testing has to be in a single thread for it to be deterministic

@jaystarshot : Could we have a single test without gauge metrics but with the multi-threading. Without that there is no signal that this code is working.

majetideepak · 2025-03-17T20:56:53Z

presto-native-execution/presto_cpp/main/runtime-metrics/PrometheusStatsReporter.h

  explicit PrometheusStatsReporter(
-      const std::map<std::string, std::string>& labels);
+      const std::map<std::string, std::string>& labels, int numThreads = 2);


Why default to 2? Should we make this configurable?

Yes we can make it configurabel

majetideepak · 2025-03-17T21:52:41Z

The StatsReporter already runs on a separate thread currently?

jaystarshot · 2025-03-17T22:31:06Z

@majetideepak The updates to in-memory metric entities currently can run within the same thread based on where its called from. #23338

This is designed to prevent any query-level slowdown, especially when handling high cardinality metrics. We previously encountered an issue where updating histograms caused latency degradation due to extremely high cardinality.
The background is that I want to add new metrics without worrying if its high cardinality or not

majetideepak · 2025-03-18T18:38:53Z

@jaystarshot #23338 got resolved with #23357

This is designed to prevent any query-level slowdown, especially when handling high cardinality metrics.

Did this PR solve the above issue from your experiments? This PR is only moving the work from one thread to another.
In a busy system, the query can still be slow if the thread updating the metrics takes long.

I am also curious if the updates can now happen out of order. Will this lead to a less precise history?

jaystarshot · 2025-03-18T18:59:12Z

@majetideepak this issue wasn’t in a busy system it was just slowing the main thread. Also having a limited threadpool for metric updation means that even when we are busy we can control degradation to only metrics updation (limited threads).
No accuracy won’t be impacted since there is no guarantee when these metrics will be pulled by the backend

jaystarshot · 2025-03-18T19:09:40Z

Also i haven't tested if it fixed that issue since that is legacy but current way of populating metric is a ticking timebomb is someone creates a high cardinality metric

majetideepak

Thanks, @jaystarshot

aditi-pandit

Thanks @jaystarshot. Had one minor question.

aditi-pandit · 2025-03-18T19:56:33Z

presto-native-execution/presto_cpp/main/runtime-metrics/PrometheusStatsReporter.cpp

@@ -241,4 +246,8 @@ std::string PrometheusStatsReporter::fetchMetrics() {
  return serializer.Serialize(impl_->registry->Collect());
 }

+void PrometheusStatsReporter::waitForCompletion() const {


Does this need to be called in the destructor as well ? Would ensure we didn't lose metrics at shutdown.

Nope we don't need in destructor. Prometheus is pull based and unless the metrics are pulled by the backend before closing that would be useless.

aditi-pandit · 2025-03-18T19:56:55Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

+    return resultOpt.value();
+  }
+  // default
+  return 2;


Nit : Abstract this with a constant variable.

aditi-pandit

Thanks @jaystarshot

jaystarshot force-pushed the jay-oss-prom branch 4 times, most recently from 73f3402 to b0c75a3 Compare March 12, 2025 18:59

jaystarshot marked this pull request as ready for review March 12, 2025 19:15

jaystarshot requested a review from a team as a code owner March 12, 2025 19:15

jaystarshot requested review from majetideepak, aditi-pandit and karteekmurthys March 12, 2025 19:16

majetideepak changed the title ~~Make prometheus metric updation async~~ [native] Update prometheus metrics asynchronously Mar 17, 2025

majetideepak reviewed Mar 17, 2025

View reviewed changes

jaystarshot force-pushed the jay-oss-prom branch 5 times, most recently from 19cf1fe to 99979d6 Compare March 18, 2025 03:07

majetideepak previously approved these changes Mar 18, 2025

View reviewed changes

aditi-pandit reviewed Mar 18, 2025

View reviewed changes

jaystarshot dismissed majetideepak’s stale review via 115db99 March 19, 2025 02:26

jaystarshot force-pushed the jay-oss-prom branch 3 times, most recently from 60afd31 to f60aa0f Compare March 19, 2025 02:31

Make prometheus metric updation async

7022794

jaystarshot force-pushed the jay-oss-prom branch from f60aa0f to 7022794 Compare March 19, 2025 03:48

aditi-pandit approved these changes Mar 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Update prometheus metrics asynchronously #24716

[native] Update prometheus metrics asynchronously #24716

jaystarshot commented Mar 12, 2025 •

edited

Loading

jaystarshot commented Mar 17, 2025

majetideepak left a comment

majetideepak Mar 17, 2025

jaystarshot Mar 17, 2025

aditi-pandit Mar 17, 2025

jaystarshot Mar 18, 2025

majetideepak Mar 17, 2025

jaystarshot Mar 17, 2025

majetideepak commented Mar 17, 2025

jaystarshot commented Mar 17, 2025 •

edited

Loading

majetideepak commented Mar 18, 2025

jaystarshot commented Mar 18, 2025 •

edited

Loading

jaystarshot commented Mar 18, 2025

majetideepak left a comment

aditi-pandit left a comment

aditi-pandit Mar 18, 2025

jaystarshot Mar 19, 2025

aditi-pandit Mar 18, 2025

aditi-pandit left a comment

[native] Update prometheus metrics asynchronously #24716

Are you sure you want to change the base?

[native] Update prometheus metrics asynchronously #24716

Conversation

jaystarshot commented Mar 12, 2025 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

jaystarshot commented Mar 17, 2025

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Mar 17, 2025

jaystarshot commented Mar 17, 2025 • edited Loading

majetideepak commented Mar 18, 2025

jaystarshot commented Mar 18, 2025 • edited Loading

jaystarshot commented Mar 18, 2025

majetideepak left a comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

jaystarshot commented Mar 12, 2025 •

edited

Loading

jaystarshot commented Mar 17, 2025 •

edited

Loading

jaystarshot commented Mar 18, 2025 •

edited

Loading