[native] Expose REST API to fetch worker stats in Prometheus format #22360

karteekmurthys · 2024-03-28T20:48:42Z

Description

Add Prometheus Reporter using the prometheus-cpp library. The library has interfaces to create all metric types (COUNTER, GAUGE, Histograms and Summaries) defined in the BaseStatsReporter. The library provides a Registry which can be used to hold the references toof the counters and a serializer interface that can convert the metrics into Prometheus Data Model.
Add a CMake flag PRESTO_ENABLE_PROMETHEUS_REPORTER to enable Prometheus Reporter.
Add REST API /v1/info/metrics to fetch the metrics from Prometheus Reporter in prometheus format. This endpoint is only enabled if the Prometheus Reporter is enabled.

Related RFC.

github-actions · 2024-03-29T18:17:55Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff bccb13c...c73c5aa.

No notifications.

jaystarshot · 2024-04-10T20:21:37Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporterTest.cpp

+#include <gtest/gtest.h>
+
+namespace facebook::presto::prometheus {
+class PrometheusReporterTest : public testing::Test {


This test will not run in the CI since that flag is off by default, can we do anything else to make it run?

We may have to create a new CI job that builds code with prometheus enabled and run this test. Or, always build the code in existing CI jobs with prometheus enabled.

Maybe just add a comment stating that this test is NoOp in CI for now

jaystarshot · 2024-04-10T20:27:40Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.h

+  // the ::prometheus::Registry allows us to Add new metric object each time
+  // which overwrites the existing metric object in its internal mapping.
+  mutable CounterMap countersMap_;
+  mutable GaugeMap gaugeMap_;


Is it better to use folly::ConcurrentHashMaps directly instead of having a custom mutex? That way concurrency will be better since all maps do not need to share a single mutex

Switched to using Synchronous<> decorator.

jaystarshot · 2024-04-10T20:39:51Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.h

+  mutable std::unordered_map<std::string, ::facebook::velox::StatType>
+      registeredMetrics_;
+  // @TODO: may be add StatType::HISTOGRAM to avoid this member.
+  mutable std::unordered_set<std::string> registeredHistogramMetrics_;


Do we need this set? Can we just use histogramMap_ instead?

Agree that we don't need it. I wanted to isolate the registered metrics Map which is modified once at startup from the actual metrics map (which can be modified concurrently).

jaystarshot · 2024-04-10T20:41:47Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

+  // If percentiles are provided, create a Summary type metric and register.
+  if (pcts.size() > 0) {
+    auto& summaryFamily = ::prometheus::BuildSummary()
+                              .Name(keyStr.append(kSummarySuffix))


Wont the metric name changed if we add a ksummarySuffix? Why is this needed?

Or is it so that maybe we don't need the histogram if pcts are present and we can just register summary?

If you check prometheus, Histograms and Summaries are 2 different metric types. If the incoming metric has quantiles (pcts) specified, then we use summary type as well.
https://prometheus.io/docs/concepts/metric_types/#histogram

https://prometheus.io/docs/practices/histograms/#:~:text=The%20essential%20difference%20between%20summaries,the%20server%20side%20using%20the
The essential difference between summaries and histograms is that summaries calculate streaming φ-quantiles on the client side and expose them directly, while histograms expose bucketed observation counts and the calculation of quantiles from the buckets of a histogram happens on the server side using the [histogram_quantile() function](https://prometheus.io/docs/prometheus/latest/querying/functions/#histogram_quantile).
So maybe we do not need to register histogram if quantiles are present?

Not sure about this^ just fyi

We can revisit this. Not sure at this point if both give different viewpoints or Summary is sufficient over histograms.

Cool thats fine, just check the kSummarySuffix i think the code here should be keyStr + "_Suffix" by value and we should not change the reference here

jaystarshot · 2024-04-10T20:47:48Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

+    case facebook::velox::StatType::SUM:
+    case facebook::velox::StatType::AVG:
+    case facebook::velox::StatType::RATE: {
+      auto& gaugeFamily =


Not entirely sure about the code style but if we don't write std::string as ::std::string then why should we write ::prometheus?

It is a required thing to resolve global namespace. Compiler complains if we remove this. I don't have an exact reason why we need it.

@karteekmurthys : Can you add "using namespace prometheus" statement to avoid this ?

jaystarshot · 2024-04-10T20:56:36Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

+  }
+  // Prometheus format requires '.' to be replaced.
+  std::string keyStr = std::string(key);
+  std::replace(keyStr.begin(), keyStr.end(), '.', '_');


I don't see the replacement of '.' being used in addMetricValue function do we need this there?

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

jaystarshot · 2024-04-10T21:57:37Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

+  // If percentiles are provided, create a Summary type metric and register.
+  if (pcts.size() > 0) {
+    auto& summaryFamily = ::prometheus::BuildSummary()
+                              .Name(keyStr.append(kSummarySuffix))


you have keyStr.append(kSummarySuffix) which means that the reference changed, then you have summaryMap_.emplace(std::string(key), summaryMetric);. So i think you are storing the key+suffix in the summary map

jaystarshot · 2024-04-10T22:22:27Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

+                              .Register(*registry_);
+    ::prometheus::Summary::Quantiles quantiles;
+    for (auto pct : pcts) {
+      quantiles.push_back(


can just use quantiles.emplace_back(pct / (double)100, 0);

jaystarshot

Added some comments + I think we need to setup a CI job for this (maybe make unittest with flag on just for this unit test temoporariliy)

aditi-pandit

Thanks @karteekmurthys. Only few minor comments.

aditi-pandit · 2024-04-25T01:05:05Z

presto-native-execution/presto_cpp/main/PrestoMain.cpp

+// Initialize singleton for the reporter
+folly::Singleton<facebook::velox::BaseStatsReporter> reporter([]() {
+  auto nodeConfig = facebook::presto::NodeConfig::instance();
+  std::string cluster = nodeConfig->nodeEnvironment();


const for cluster, hostName and worker variables.

aditi-pandit · 2024-04-25T01:06:30Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -1093,6 +1109,10 @@ void PrestoServer::populateMemAndCPUInfo() {

  // Fill global pool fields.
  poolInfo.maxBytes = nodeMemoryGb * 1024 * 1024 * 1024;
+  // TODO(sperhsin): If 'current bytes' is the same as we get by summing up all


Nit spelling spershin

aditi-pandit · 2024-04-25T01:06:51Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -1146,6 +1165,20 @@ void PrestoServer::reportServerInfo(proxygen::ResponseHandler* downstream) {
  http::sendOkResponse(downstream, json(serverInfo));
 }

+void PrestoServer::reportWorkerMetrics(proxygen::ResponseHandler* downstream) {
+  auto nodeConfig = facebook::presto::NodeConfig::instance();
+  std::string cluster = nodeConfig->nodeEnvironment();


aditi-pandit · 2024-04-25T01:07:35Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

+  char* hostName = std::getenv("HOSTNAME");
+  std::string worker = !hostName ? "" : hostName;
+#ifdef PRESTO_ENABLE_PROMETHEUS_REPORTER
+  auto reporter = std::dynamic_pointer_cast<


Maybe all variables above should be within #ifdef. Else it can be empty.

aditi-pandit · 2024-04-25T01:09:04Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -328,6 +336,14 @@ void PrestoServer::run() {
          proxygen::ResponseHandler* downstream) {
        server->reportServerInfo(downstream);
      });
+  httpServer_->registerGet(


It seems like this will always be registered irrespective of whether you are using Prometheus. Is that desired ? We could avoid it in the Meta environment where they are not using Prometheus.

For now it is registered and if the Prometheus is not enabled it is a no-op.

It is odd to make it a no-op. It is better to return an invalid HTTP request error than an empty result. The later would be confusing.
Let's move the above if(systemConfig->enableRuntimeMetricsCollection()) check here and include this.

aditi-pandit · 2024-04-25T01:14:43Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

+  auto bound = min + bucketWidth;
+  std::string keyStr = std::string(key);
+  std::replace(keyStr.begin(), keyStr.end(), '.', '_');
+  auto& histogramFamily =


Nit : Add newline before

aditi-pandit · 2024-04-25T01:17:23Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

+}
+
+void PrometheusReporter::registerHistogramMetricExportType(
+    folly::StringPiece key,


Why not const& ?

Defined in interface.

aditi-pandit · 2024-04-25T01:22:50Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporterTest.cpp

+      "test_key4{" + labelsSerialized + "} 0"};
+
+  auto verifySerializedResult = [&](std::string& fullSerializedResult,
+                                    std::vector<std::string>& expected) {


aditi-pandit · 2024-04-25T01:23:23Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporterTest.cpp

+      "# TYPE test_key4 gauge",
+      "test_key4{" + labelsSerialized + "} 0"};
+
+  auto verifySerializedResult = [&](std::string& fullSerializedResult,


I feel you should send a const& here and make a copy in the verify routine. It makes it more readable.

aditi-pandit · 2024-04-25T01:24:18Z

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporterTest.cpp

+      "# TYPE test_key4 gauge",
+      "test_key4{" + labelsSerialized + "} 0"};
+
+  auto verifySerializedResult = [&](std::string& fullSerializedResult,


You can even make this a separate function instead of a lambda

czentgr

A few comments related to build integration.

presto-native-execution/scripts/setup-adapters.sh

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp

presto-native-execution/CMakeLists.txt

majetideepak

@karteekmurthys some high-level comments.

presto-native-execution/presto_cpp/main/PrestoMain.cpp

majetideepak · 2024-05-10T13:28:15Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -328,6 +336,14 @@ void PrestoServer::run() {
          proxygen::ResponseHandler* downstream) {
        server->reportServerInfo(downstream);
      });
+  httpServer_->registerGet(


It is odd to make it a no-op. It is better to return an invalid HTTP request error than an empty result. The later would be confusing.
Let's move the above if(systemConfig->enableRuntimeMetricsCollection()) check here and include this.

presto-native-execution/presto_cpp/main/PrestoServer.cpp

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.h

majetideepak · 2024-06-27T15:57:03Z

@tanjialiang, @amitkdutta Can you make another pass? All your feedback has been addressed.

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

majetideepak · 2024-06-28T17:31:12Z

@amitkdutta, please import this PR and let us know if you see any other issues inside Meta. Thanks!

facebook-github-bot · 2024-06-29T01:52:29Z

@amitkdutta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

amitkdutta

Looks great to me. The only issue is renaming the virtual function from enableRuntimeMetricReporting to enableWorkerStatsReporting, which we can fix inside Meta.

Thanks @karteekmurthys for all the iterations.

majetideepak · 2024-06-29T06:31:39Z

@karteekmurthys Let's fix the commit name to match the new naming
[Native] Expose REST API to fetch worker stats in Prometheus format
Let's also add the following description to the commit.

Add Prometheus Reporter using the prometheus-cpp library.
Add a CMake flag PRESTO_ENABLE_PROMETHEUS_REPORTER to enable Prometheus Reporter.
Add REST API '/v1/info/metrics' to fetch worker metrics from Prometheus Reporter in prometheus format.
This endpoint is only enabled if the Prometheus Reporter is enabled.

karteekmurthys · 2024-07-01T04:56:47Z

@amitkdutta @tanjialiang Thanks for the review.

majetideepak · 2024-07-01T15:55:23Z

@tdcmeehan can you please approve this PR? The circleci change requires a committer approval. Thanks!

lingbin · 2024-07-01T16:44:59Z

nit: Should use "[native]"(lowercase) as a tag instead of "[Native]".

tdcmeehan · 2024-07-01T16:54:49Z

Should we add documentation on how to enable reporting?

karteekmurthys · 2024-07-01T17:34:34Z

Should we add documentation on how to enable reporting?

I have opened another PR for that https://github.com/prestodb/presto/pull/23107/files.
We can iterate over documentation there.

lingbin

Looking forward to this feature.

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

lingbin · 2024-07-01T17:53:21Z

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

+    size_t value) const {
+  auto metricIterator = registeredMetricsMap_.find(key);
+  if (metricIterator == registeredMetricsMap_.end()) {
+    VLOG(1) << "addMetricValue for unregistered metric " << key;


addMetricValue -> addHistogramMetricValue

Add Prometheus Reporter using the prometheus-cpp library. Add a CMake flag PRESTO_ENABLE_PROMETHEUS_REPORTER to enable Prometheus Reporter. Add REST API '/v1/info/metrics' to fetch worker metrics from Prometheus Reporter in prometheus format. This endpoint is only enabled if the Prometheus Reporter is enabled. Co-authored-by:jaystarshot <jay.narale@uber.com>

presto-native-execution/presto_cpp/main/PrestoServer.h

karteekmurthys self-assigned this Mar 28, 2024

karteekmurthys requested a review from a team as a code owner March 28, 2024 20:48

karteekmurthys requested review from majetideepak and aditi-pandit March 28, 2024 20:48

karteekmurthys force-pushed the prestissimo-metrics branch 3 times, most recently from f218fc1 to 6470454 Compare March 29, 2024 18:13

karteekmurthys requested a review from jaystarshot March 29, 2024 18:14

karteekmurthys force-pushed the prestissimo-metrics branch from 6470454 to 711cfb6 Compare March 29, 2024 18:27

jaystarshot reviewed Apr 10, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp Outdated Show resolved Hide resolved

jaystarshot reviewed Apr 10, 2024

View reviewed changes

jaystarshot requested changes Apr 11, 2024

View reviewed changes

karteekmurthys mentioned this pull request Apr 11, 2024

[WIP] Expose Rest API to return metrics in prometheus format #21599

Closed

6 tasks

karteekmurthys force-pushed the prestissimo-metrics branch from 711cfb6 to a4a7bd3 Compare April 15, 2024 17:11

aditi-pandit reviewed Apr 25, 2024

View reviewed changes

karteekmurthys force-pushed the prestissimo-metrics branch 4 times, most recently from 22c1820 to 12302f1 Compare April 27, 2024 05:54

czentgr reviewed May 3, 2024

View reviewed changes

presto-native-execution/scripts/setup-adapters.sh Outdated Show resolved Hide resolved

presto-native-execution/presto_cpp/main/common/prometheus-metrics/PrometheusReporter.cpp Outdated Show resolved Hide resolved

presto-native-execution/CMakeLists.txt Show resolved Hide resolved

majetideepak reviewed May 10, 2024

View reviewed changes

karteekmurthys force-pushed the prestissimo-metrics branch from 12302f1 to 1d56b00 Compare May 15, 2024 23:12

karteekmurthys force-pushed the prestissimo-metrics branch from fdf16dc to 2600215 Compare June 27, 2024 17:59

majetideepak reviewed Jun 28, 2024

View reviewed changes

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

majetideepak reviewed Jun 28, 2024

View reviewed changes

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

majetideepak reviewed Jun 28, 2024

View reviewed changes

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

karteekmurthys force-pushed the prestissimo-metrics branch from 2600215 to b3a24cc Compare June 28, 2024 15:59

amitkdutta approved these changes Jun 29, 2024

View reviewed changes

majetideepak approved these changes Jun 29, 2024

View reviewed changes

majetideepak changed the title ~~[Native] Expose REST API to fetch runtime metrics in prometheus format~~ [Native] Expose REST API to fetch worker stats in prometheus format Jun 29, 2024

majetideepak changed the title ~~[Native] Expose REST API to fetch worker stats in prometheus format~~ [Native] Expose REST API to fetch worker stats in Prometheus format Jun 29, 2024

karteekmurthys force-pushed the prestissimo-metrics branch from b3a24cc to 7e49c3f Compare July 1, 2024 10:54

majetideepak approved these changes Jul 1, 2024

View reviewed changes

lingbin reviewed Jul 1, 2024

View reviewed changes

karteekmurthys changed the title ~~[Native] Expose REST API to fetch worker stats in Prometheus format~~ [native] Expose REST API to fetch worker stats in Prometheus format Jul 1, 2024

karteekmurthys force-pushed the prestissimo-metrics branch from 7e49c3f to c73c5aa Compare July 1, 2024 18:05

tdcmeehan self-assigned this Jul 1, 2024

lingbin reviewed Jul 1, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/PrestoServer.h Show resolved Hide resolved

tdcmeehan approved these changes Jul 2, 2024

View reviewed changes

majetideepak merged commit fd57488 into prestodb:master Jul 3, 2024
59 checks passed

karteekmurthys mentioned this pull request Jul 30, 2024

PrestoExchangeSource HttpClient histogram metric is blocking query #23338

Open

tdcmeehan mentioned this pull request Aug 23, 2024

Add release notes for 0.289 #23513

Merged

34 tasks

[native] Expose REST API to fetch worker stats in Prometheus format #22360

[native] Expose REST API to fetch worker stats in Prometheus format #22360

Conversation

karteekmurthys commented Mar 28, 2024 • edited by majetideepak Loading

Description

github-actions bot commented Mar 29, 2024 • edited Loading

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

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

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

jaystarshot Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaystarshot 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

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

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

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Jun 27, 2024

majetideepak commented Jun 28, 2024

facebook-github-bot commented Jun 29, 2024

amitkdutta left a comment

Choose a reason for hiding this comment

majetideepak commented Jun 29, 2024 • edited Loading

karteekmurthys commented Jul 1, 2024

majetideepak commented Jul 1, 2024

lingbin commented Jul 1, 2024

tdcmeehan commented Jul 1, 2024

karteekmurthys commented Jul 1, 2024

lingbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karteekmurthys commented Mar 28, 2024 •

edited by majetideepak

Loading

github-actions bot commented Mar 29, 2024 •

edited

Loading

jaystarshot Apr 10, 2024 •

edited

Loading

majetideepak commented Jun 29, 2024 •

edited

Loading