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

karteekmurthys · 2023-12-22T23:33:35Z

Description

This PR is still a work-in-progress.
There are 2 approaches implemented in this PR:

This PR implements the BaseStatsReporter interface as StatsReporterImpl. The current design of StatsReporterImpl holds the metrics in-memory and exposes a method getMetrics(MetricsSerializer) that serializes the metrics with the serializer passed down to the function call. MetricsSerializer is an interface that users can implement to customize metric serialization. This approach doesn't support Histogram
Implements PrometheusReporter using the prometheus-cpp library. The library has interfaces to create all metric types (COUNTER, GAUGE, Histograms and Summaries). The library exposes a Registry which can be used to hold the references to the counters. Also, a textSerializer interface that can convert the metrics into Prometheus Data Model.

We may chose one of the above or both.

To the outside world prestissimo exposes a rest API /v1/info/health/metrics, which in turn uses StatsReporterImpl::getMetrics(MetricsSerializer).

The Prometheus server must be configured to regularly probe this Prestissimo rest API to collect metrics.

Test Plan

Added a unit test class for StatsReporterImpl and also PrometheusReporter.
Also, Tested REST API locally and results are attached here.
prometheus-format-metrics.txt using prometheus-cpp library

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
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.

== NO RELEASE NOTE ==

linux-foundation-easycla · 2023-12-22T23:33:39Z

✅ login: karteekmurthys / name: Karteek (1d279f8)
❌ The email address for the commit (49992f3, 4d489a4, c739d83, 5c510e7, 3ce8d6c) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

tdcmeehan · 2023-12-23T01:15:00Z

Ahana is not a great name for this. How about PrometheusStatsReporter?

aditi-pandit · 2023-12-27T01:58:43Z

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

@@ -994,6 +1003,11 @@ void PrestoServer::reportServerInfo(proxygen::ResponseHandler* downstream) {
  http::sendOkResponse(downstream, json(serverInfo));
 }

+void PrestoServer::reportHealthMetrics(proxygen::ResponseHandler* downstream) {
+  auto reporter = std::dynamic_pointer_cast<ahana::PrometheusStatsReporter>(


Remove the ahana namespace.

aditi-pandit · 2023-12-27T02:00:51Z

presto-native-execution/presto_cpp/main/tests/PrometheusStatsReporterTest.cpp

+
+namespace ahana {
+
+class AhanaStatsReporterTest : public testing::Test {


Remove the use of the ahana namespace and use PrometheusStatsReporterTest as the name of the test instead.

aditi-pandit · 2023-12-27T02:02:06Z

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

+
+#include "PrometheusStatsReporter.h"
+
+namespace ahana {


This class should be under facebook::presto namespace.

aditi-pandit · 2023-12-27T02:04:19Z

presto-native-execution/presto_cpp/main/common/PrometheusStatsReporter.h

+      const std::vector<int32_t>& /* pcts */) const override {}
+
+  void addMetricValue(const std::string& key, size_t value = 1)
+      const override{};


Why is this an empty method ? We should do the same logic as the other addMetricValue methods.

aditi-pandit · 2023-12-27T19:32:09Z

@karteekmurthys : Please add some documentation. The doc should have information about:

Background on Prometheus metrics. What do gauge and counter metrics mean ? Add some screenshots to help the reviewer understand.
How do Prestissimo metrics map to Prometheus metrics ? Give some examples of metrics that are mapped.
How to setup this ?

In addition, I feel that Prometheus StatsReporter should be added based on some config (and not default).

steveburnett · 2024-01-02T18:41:02Z

@karteekmurthys let me know when you have added the documentation and I'll review it then. If I can help you get the docs drafted into review-ready status, just ask!

karteekmurthys · 2024-01-02T18:42:42Z

@karteekmurthys let me know when you have added the documentation and I'll review it then. If I can help you get the docs drafted into review-ready status, just ask!

@steveburnett thanks! I am currently drafting it. Will reach out to you soon.

cc: @aditi-pandit

jaystarshot · 2024-01-17T01:34:06Z

presto-native-execution/presto_cpp/main/common/PrometheusStatsReporter.h

+
+ private:
+  /// Mapping of registered stats key to StatType.
+  mutable std::unordered_map<std::string, facebook::velox::StatType>


Have you considered using https://github.com/jupp0r/prometheus-cpp or some opensource implementation for stats collection? If not any specific reason?

The data model is not too complex to rely on a library at this point. We want to keep external dependencies minimal as well. If we discover that writing serialization ourselves is getting complex we may consider using the library.
The library has additional support to create exporter as well as a push support to a gateway.

void RegisterCollectable(const std::weak_ptr<Collectable>& collectable, const std::string& uri = std::string("/metrics")); void RegisterAuth( std::function<bool(const std::string&, const std::string&)> authCB, const std::string& realm = "Prometheus-cpp Exporter", const std::string& uri = std::string("/metrics"));

I don't think we need these capabilities at this point.

On the other hand, it supports all metrics, is production tested and widely used (though like an unofficial library)

I have created a draft PR - #21753, we believe that using the library is the fastest way to production since its production tested. Not sure how to integrate

Using the library seems better if it is production-ready and widely used.
This can be an optional dependency that we can turn on/off at compile time.
I can help with the dependency management.

@karteekmurthys I believe Jay already has a PR in progress here #21753
We should base our work on that.
You can help with the dependency management in that PR.

@jaystarshot I took a shot at using prometheus-cpp in this PR itself. I have added a compile time settings to include prometheus-directory. Added some unit tests. I will further modify the circle ci configs to run unit tests for this change. Feel free to use this PR for your testing. I will be opening an RFC soon to get this merged with Presto.

Have you tried this in local? I don't think this will work since there is no exposer defined. My PR #21753 shows how to do that. Can you share the RFC with me too? I can also input from my end thanks

We don't need an Exposer. We already expose a REST API, check PrestoServer.cpp changes in this PR.
You can configure your Prometheus server to scrape the enpoint: /v1/info/health/metrics exposed by the worker.
Or you can configure whatever intermediary process to scrape from worker endpoint.
Missed adding the test results in previous comment, here it is: prometheus-format-metrics.txt

I see thanks! I think having an exposer is better since it separates the http server and makes it configurable based on the number of threads etc. In future we can also move this easily to a sidecar. We can sync offline regarding the RFC too if you have a draft prepared already

majetideepak · 2024-01-20T03:40:14Z

Dockerfile-native-debug

@@ -0,0 +1,52 @@
+ARG PRESTO_VERSION


let's move the Docker/Jenkins changes to its own PR.

amitkdutta · 2024-01-22T02:20:41Z

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

@@ -210,6 +211,10 @@ void PrestoServer::run() {
    exit(EXIT_FAILURE);
  }

+  if (systemConfig->enableRuntimeStatsCollection()) {


@karteekmurthys Karteek, let me add this config in a separte PR. Inside Meta there is a small wrapper on PrestoServer that does following:

if (gflag enabled): initialize stats reporting initialize presto server

I think we can use the config and remove the internal flag altogether. Let me introduce this flag in a PR and modify internal code. You can rebase on top of that.

@karteekmurthys Here is the small PR
#21745
Once we merge it, I will refactor internal logic to use the cofnig property.

jaystarshot · 2024-01-31T21:52:49Z

presto-native-execution/presto_cpp/main/common/StatsReporterImpl.h

+
+ private:
+  /// Mapping of registered stats key to StatType.
+  mutable std::unordered_map<std::string, facebook::velox::StatType>


Use mutable folly::ConcurrentHashMap

we normally use folly::Synchronized<> to do this. Lmk revisit this after finishing the RFC. I wanted to unblock any testing you wanted to do, so focussed on getting a working version. I need to clean up the tests as well.

jaystarshot · 2024-01-31T21:53:03Z

presto-native-execution/presto_cpp/main/common/StatsReporterImpl.h

+  /// A mapping from stats key of type COUNT to value.
+  mutable std::unordered_map<std::string, size_t> metricsMap_;
+  // Mutex to control access to registeredStats_ and metricMap_ members.
+  mutable std::mutex mutex_;


Do not need mutex

jaystarshot · 2024-01-31T22:05:35Z

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

+          proxygen::HTTPMessage* /*message*/,
+          const std::vector<std::unique_ptr<folly::IOBuf>>& /*body*/,
+          proxygen::ResponseHandler* downstream) {
+        server->reportHealthMetrics(downstream);


Why do we need this? If we just define an exposer that will create another http server on /metrics or /v1/metrics? We can also define the number of threads to be used by the new http server

That is exactly what we are trying to avoid. We didn't find a strong need to start a new server sharing space with our worker containers. Reduces the overhead of maintaining and launching another process. Why do you want multiple threads to be spawned by the exposer? are you expecting high traffic.
CMIW, prometheus server is configured to periodically call the scrape endpoint. So, we can expect 1 HTTP request at X seconds interval. If you are expecting huge traffic, then it is not recommended to share space with worker instance.

I think isolation is also one point, we do not want to degrade the presto process as much as we can

@amitkdutta : What approach did Meta take for this ? Does your metric collection use any endpoint in Presto worker process itself, or did you start another server ?

karteekmurthys · 2024-04-11T06:01:19Z

Moved it here: #22360 (review)

karteekmurthys requested a review from a team as a code owner December 22, 2023 23:33

karteekmurthys self-assigned this Dec 22, 2023

karteekmurthys requested review from ethanyzhang, majetideepak and aditi-pandit December 22, 2023 23:33

karteekmurthys force-pushed the prestissimo-metrics branch from bc7f6a3 to 87e94ee Compare December 22, 2023 23:43

karteekmurthys force-pushed the prestissimo-metrics branch from 87e94ee to 1cabfe5 Compare December 23, 2023 16:43

aditi-pandit reviewed Dec 27, 2023

View reviewed changes

karteekmurthys force-pushed the prestissimo-metrics branch 2 times, most recently from bdff158 to 13e58ac Compare December 27, 2023 07:34

karteekmurthys changed the title ~~Expose Rest API to return metrics in prometheus format~~ [WIP] Expose Rest API to return metrics in prometheus format Dec 27, 2023

karteekmurthys marked this pull request as draft December 27, 2023 07:55

tdcmeehan requested a review from steveburnett January 2, 2024 14:59

tdcmeehan mentioned this pull request Jan 2, 2024

Do we have existing Prometheus implementation for Velox BaseStatsReporter? #21615

Closed

jaystarshot self-requested a review January 3, 2024 07:34

wanglinsong force-pushed the native-worker-image branch from 7b8851f to b01736d Compare January 3, 2024 23:25

presto-release-bot force-pushed the native-worker-image branch 4 times, most recently from daca1b2 to 0effee9 Compare January 11, 2024 02:00

karteekmurthys force-pushed the prestissimo-metrics branch from 13e58ac to 4dc5230 Compare January 13, 2024 00:47

presto-release-bot force-pushed the native-worker-image branch from 6e46e4d to 725c364 Compare January 15, 2024 02:00

jaystarshot reviewed Jan 17, 2024

View reviewed changes

presto-release-bot force-pushed the native-worker-image branch from 725c364 to 892525f Compare January 17, 2024 02:01

karteekmurthys force-pushed the prestissimo-metrics branch from 4dc5230 to e16ad47 Compare January 18, 2024 18:41

karteekmurthys force-pushed the prestissimo-metrics branch from e16ad47 to a4dc38c Compare January 18, 2024 19:16

presto-release-bot force-pushed the native-worker-image branch from 892525f to 571c2c8 Compare January 19, 2024 02:00

majetideepak reviewed Jan 20, 2024

View reviewed changes

ethanyzhang force-pushed the native-worker-image branch 2 times, most recently from d0e3abb to 663fe40 Compare January 20, 2024 05:37

presto-release-bot force-pushed the native-worker-image branch from 076f229 to f1fb59f Compare January 21, 2024 02:01

amitkdutta reviewed Jan 22, 2024

View reviewed changes

amitkdutta mentioned this pull request Jan 22, 2024

[native] Add config for stats reporting and counters. #21745

Merged

karteekmurthys force-pushed the prestissimo-metrics branch from 1f43cb8 to 0046c30 Compare January 23, 2024 18:14

karteekmurthys changed the base branch from native-worker-image to master January 23, 2024 18:54

karteekmurthys force-pushed the prestissimo-metrics branch from 0046c30 to 01bb923 Compare January 31, 2024 19:53

karteekmurthys and others added 6 commits January 31, 2024 13:02

Expose Rest API to return metrics in prometheus format

1d279f8

Overwrite gauge metrics instead of batching them

49992f3

Refactor PrometheusStatsReporter to StatsReporterImpl"

4d489a4

Add new Serializer interface

c739d83

Prometheus serializer

5c510e7

[Native] Prometheus exporter using prometheus-cpp client

3ce8d6c

karteekmurthys force-pushed the prestissimo-metrics branch from 01bb923 to 3ce8d6c Compare January 31, 2024 21:12

jaystarshot reviewed Jan 31, 2024

View reviewed changes

karteekmurthys closed this Apr 11, 2024

ethanyzhang deleted the prestissimo-metrics branch July 8, 2024 02:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

karteekmurthys commented Dec 22, 2023 •

edited

Loading

linux-foundation-easycla bot commented Dec 22, 2023 •

edited

Loading

tdcmeehan commented Dec 23, 2023

aditi-pandit Dec 27, 2023

aditi-pandit Dec 27, 2023

aditi-pandit Dec 27, 2023

aditi-pandit Dec 27, 2023

aditi-pandit commented Dec 27, 2023

steveburnett commented Jan 2, 2024

karteekmurthys commented Jan 2, 2024

jaystarshot Jan 17, 2024

karteekmurthys Jan 17, 2024

jaystarshot Jan 18, 2024

jaystarshot Jan 22, 2024

majetideepak Jan 23, 2024

majetideepak Jan 23, 2024

karteekmurthys Jan 31, 2024

jaystarshot Jan 31, 2024

karteekmurthys Jan 31, 2024 •

edited

Loading

jaystarshot Jan 31, 2024

majetideepak Jan 20, 2024

amitkdutta Jan 22, 2024

amitkdutta Jan 22, 2024

jaystarshot Jan 31, 2024

karteekmurthys Jan 31, 2024

jaystarshot Jan 31, 2024

jaystarshot Jan 31, 2024

karteekmurthys Jan 31, 2024

jaystarshot Jan 31, 2024

aditi-pandit Feb 8, 2024

karteekmurthys commented Apr 11, 2024


		namespace ahana {

		class AhanaStatsReporterTest : public testing::Test {

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

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

Conversation

karteekmurthys commented Dec 22, 2023 • edited Loading

Description

Test Plan

Contributor checklist

linux-foundation-easycla bot commented Dec 22, 2023 • edited Loading

tdcmeehan commented Dec 23, 2023

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 commented Dec 27, 2023

steveburnett commented Jan 2, 2024

karteekmurthys commented Jan 2, 2024

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

karteekmurthys Jan 31, 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

karteekmurthys commented Apr 11, 2024

karteekmurthys commented Dec 22, 2023 •

edited

Loading

linux-foundation-easycla bot commented Dec 22, 2023 •

edited

Loading

karteekmurthys Jan 31, 2024 •

edited

Loading