stats: add prometheus formatted stats in the admin endpoint #2026

lita · 2017-11-08T22:56:08Z

Description: Adding /metric endpoint that returns prometheus formatted stats described here. This endpoint only handles counters and gauges.

Part of #1947

Risk Level: Low

Testing: integration tests

Signed-off-by: Lita Cho [email protected]

mattklein123 · 2017-11-08T23:04:01Z

@ccaraman can you help @lita with initial review on this. Thank you @lita! This will be very popular.

ccaraman · 2017-11-08T23:43:41Z

docs/operations/admin.rst

@@ -141,3 +141,9 @@ The fields are:
  .. http:get:: /stats?format=json

  Outputs /stats in JSON format. This can be used for programmatic access of stats.
+
+.. http:get:: /metrics


when building the docs, did this format correctly? The indentation is off with the rest of the admin endpoints.

Here are the docs to build docs https://github.com/envoyproxy/envoy/tree/master/docs#developer-local-docs-build

Also keep the alphabetized order for the methods.

ccaraman · 2017-11-08T23:45:37Z

source/server/http/admin.h

@@ -130,6 +133,7 @@ class AdminImpl : public Admin,
  Http::Code handlerResetCounters(const std::string& url, Buffer::Instance& response);
  Http::Code handlerServerInfo(const std::string& url, Buffer::Instance& response);
  Http::Code handlerStats(const std::string& url, Buffer::Instance& response);
+  Http::Code handlePrometheusStats(const std::string& url, Buffer::Instance& response);


Change the method name to match the admin method.

and typo in name handle-> handler

ccaraman · 2017-11-08T23:46:07Z

source/server/http/admin.cc

@@ -412,6 +449,8 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof
          {"/server_info", "print server version/status information",
           MAKE_ADMIN_HANDLER(handlerServerInfo), false},
          {"/stats", "print server stats", MAKE_ADMIN_HANDLER(handlerStats), false},
+          {"/metrics", "print server stats in prometheus' format",


nit: trailing ` after prometheus

ccaraman · 2017-11-08T23:48:26Z

source/server/http/admin.cc

@@ -311,6 +311,43 @@ Http::Code AdminImpl::handlerStats(const std::string& url, Buffer::Instance& res
  return rc;
 }

+Http::Code AdminImpl::handlePrometheusStats(const std::string& url, Buffer::Instance& response) {
+  // As stated above, the /metric endpoint also doesn't support timers locally (only via statsd)
+  Http::Code rc = Http::Code::OK;


This line isn't needed since rc doesn't get updated by subsequent calls.

ccaraman · 2017-11-08T23:50:29Z

source/server/http/admin.cc

@@ -311,6 +311,43 @@ Http::Code AdminImpl::handlerStats(const std::string& url, Buffer::Instance& res
  return rc;
 }

+Http::Code AdminImpl::handlePrometheusStats(const std::string& url, Buffer::Instance& response) {
+  // As stated above, the /metric endpoint also doesn't support timers locally (only via statsd)


nit: can you be more explicit about where above is. I think you are referring to the handlerStats endpoint comment.

ccaraman · 2017-11-08T23:51:26Z

source/server/http/admin.cc

+Http::Code AdminImpl::handlePrometheusStats(const std::string& url, Buffer::Instance& response) {
+  // As stated above, the /metric endpoint also doesn't support timers locally (only via statsd)
+  Http::Code rc = Http::Code::OK;
+  const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url);


Not needed.

ggreenway · 2017-11-09T00:02:14Z

source/server/http/admin.cc

+Http::Code AdminImpl::handlePrometheusStats(const std::string& url, Buffer::Instance& response) {
+  // As stated above, the /metric endpoint also doesn't support timers locally (only via statsd)
+  Http::Code rc = Http::Code::OK;
+  const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url);


This is unused. Delete?

ggreenway · 2017-11-09T00:05:11Z

source/server/http/admin.cc

+                                  Buffer::Instance& response) {
+  std::string comma = ",";
+  for (const Stats::CounterSharedPtr& counter : counters) {
+    std::string tags;


Please add a helper function for the common body of these two loops.

ggreenway · 2017-11-09T00:16:54Z

source/server/http/admin.cc

+    }
+    response.add(fmt::format("# TYPE {0} counter\n", counter->tagExtractedName()));
+    response.add(
+        fmt::format("{0}{{{1}}} {2}\n", counter->tagExtractedName(), tags, counter->value()));


I think you may need to do some escaping here. I can't seem to find the docs, but I'm pretty sure that the restrictions on names in the format are:

names can contain letters and digits and _ but no other characters

names cannot begin with a digit

Someone please correct me if I'm wrong on that.

This is the link you have been looking for: https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels

Thanks! That's exactly what I was thinking of (but failed to find).

Here is the testing output of the stats: https://gist.github.com/lita/8f145f87d4390e050ea4b36951f359ab

From what I can tell, it seems to follow the metric naming rules for Prometheus.

htuch · 2017-11-13T02:58:06Z

source/server/http/admin.cc

+      tags += sep;
+      tags += fmt::format("{}={}", tag.name_, tag.value_);
+      sep = comma;
+    }


It seems that this could be more cleanly expressed as a map-join operation.

htuch · 2017-11-13T03:02:27Z

source/server/http/admin.cc

@@ -412,6 +449,8 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof
          {"/server_info", "print server version/status information",
           MAKE_ADMIN_HANDLER(handlerServerInfo), false},
          {"/stats", "print server stats", MAKE_ADMIN_HANDLER(handlerStats), false},
+          {"/metrics", "print server stats in prometheus' format",


I can't remember whether we've had this conversation; would it make sense to have all the different stats format under /stats, so similar to JSON we would have /stats?format=prometheus? Just raising the question for discussion, since I can anticipate further stats format, are we going to look for more synonyms for "stats" when that happens? :)

This is the way it used to be, but we changed it back to /metrics because that is the defualt Prometheus path. In thinking about it, I agree with @htuch that we should switch it back to /stats?format=prometheus. We are not a Prometheus only operation so this is more consistent and it will be well documented.

That's fine. I can change it back.

mattklein123 · 2017-11-20T18:12:18Z

@lita friendly ping, can we move this forward? If not for a while we can close until there is time to work on it?

pschultz · 2017-11-20T18:22:05Z

I've been messing with the statsd exporter for now, so this is a very welcome feature.

I noticed that many of the statsd timing values are in milliseconds. Are the values from the /metrics endpoint scaled to base units (e.g. seconds, not milliseconds), as suggested in the best practices?

lita · 2017-11-20T18:35:28Z

@mattklein123 yup yup, I am adding the changes for map/joins and changing it back to /stats=prometheus. Should have an updated PR this week.

@pschultz, I am not making any changes to the timestamp right now. Maybe in the future, we can add params to return seconds if necessary, but it seems out of scope for this PR since the other stat formats (json) don't do that.

pschultz · 2017-11-20T18:39:28Z

Providing the values in base units is best practice for Prometheus metrics. Given that you are implementing Prometheus metrics it seems very much in scope to me.

mattklein123 · 2017-11-20T18:43:09Z

@pschultz there are no timing values as part of this change. Just counters and gauges. When we eventually add direct output of timing, etc. we will consider the right units to output.

lita · 2017-11-26T05:23:49Z

Here is an example output of the endpoint based on the test data: https://gist.github.com/lita/8f145f87d4390e050ea4b36951f359ab

lita · 2017-11-26T08:02:28Z

I have created the data-plane-api PR with my docs: envoyproxy/data-plane-api#268

mattklein123

nice. LGTM. a few nits.

mattklein123 · 2017-11-26T18:56:18Z

source/server/http/admin.cc

@@ -311,6 +313,31 @@ Http::Code AdminImpl::handlerStats(const std::string& url, Buffer::Instance& res
  return rc;
 }

+std::string AdminImpl::formatTagsForPrometheus(const std::vector<Stats::Tag>& tags) {
+  std::vector<std::string> buf;
+  for (const Stats::Tag tag : tags) {


const Stats::Tag&

mattklein123 · 2017-11-26T18:56:31Z

source/server/http/admin.cc

+void AdminImpl::statsAsPrometheus(const std::list<Stats::CounterSharedPtr>& counters,
+                                  const std::list<Stats::GaugeSharedPtr>& gauges,
+                                  Buffer::Instance& response) {
+  for (auto counter : counters) {


const auto&

mattklein123 · 2017-11-26T18:56:39Z

source/server/http/admin.cc

+                                  const std::list<Stats::GaugeSharedPtr>& gauges,
+                                  Buffer::Instance& response) {
+  for (auto counter : counters) {
+    std::string tags = formatTagsForPrometheus(counter->tags());


mattklein123 · 2017-11-26T18:56:49Z

source/server/http/admin.cc

+        fmt::format("{0}{{{1}}} {2}\n", counter->tagExtractedName(), tags, counter->value()));
+  }
+
+  for (auto gauge : gauges) {


const auto&

mattklein123 · 2017-11-26T18:56:54Z

source/server/http/admin.cc

+  }
+
+  for (auto gauge : gauges) {
+    std::string tags = formatTagsForPrometheus(gauge->tags());


mattklein123 · 2017-11-26T18:57:06Z

source/server/http/admin.h

@@ -114,6 +114,10 @@ class AdminImpl : public Admin,
                      const Upstream::Outlier::Detector* outlier_detector,
                      Buffer::Instance& response);
  std::string statsAsJson(const std::map<std::string, uint64_t>& all_stats);
+  void statsAsPrometheus(const std::list<Stats::CounterSharedPtr>& counters,


nit: I think this can be static

Signed-off-by: Lita Cho <[email protected]>

mattklein123

nice, thanks!

This commit updates the Envoy SHA to 74de08a to bring in the new TCP RBAC filter to Istio. Signed-off-by: Venil Noronha <[email protected]>

mattklein123 requested a review from ccaraman November 8, 2017 23:03

lita force-pushed the prometheus-stats branch from ac1eb40 to d72f7eb Compare November 8, 2017 23:31

ccaraman reviewed Nov 9, 2017

View reviewed changes

ggreenway requested changes Nov 9, 2017

View reviewed changes

htuch reviewed Nov 13, 2017

View reviewed changes

lita force-pushed the prometheus-stats branch 2 times, most recently from 61d03ac to 344c98a Compare November 26, 2017 04:58

lita force-pushed the prometheus-stats branch from 344c98a to 9fa68bd Compare November 26, 2017 06:47

lita mentioned this pull request Nov 26, 2017

docs: add docs for prometheus stats envoyproxy/data-plane-api#268

Merged

mattklein123 reviewed Nov 26, 2017

View reviewed changes

Add Prometheus Formatted Stats to Admin Endpoints

43f6f24

Signed-off-by: Lita Cho <[email protected]>

lita force-pushed the prometheus-stats branch from 9fa68bd to 43f6f24 Compare November 26, 2017 23:09

mattklein123 approved these changes Nov 26, 2017

View reviewed changes

mattklein123 merged commit 6e36334 into envoyproxy:master Nov 26, 2017

JonathanO mentioned this pull request Nov 28, 2017

Prometheus format metrics aren't parsable by Prometheus #2118

Closed

ccmtaylor mentioned this pull request Dec 11, 2017

stats: expose Prometheus metrics as /metrics #2182

Closed

grantr mentioned this pull request Feb 26, 2018

Cheap and cheerful autoscaler knative/serving#229

Merged

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Update Envoy SHA to 74de08a (envoyproxy#2026)

65087d0

This commit updates the Envoy SHA to 74de08a to bring in the new TCP RBAC filter to Istio. Signed-off-by: Venil Noronha <[email protected]>

stats: add prometheus formatted stats in the admin endpoint #2026

stats: add prometheus formatted stats in the admin endpoint #2026

Conversation

lita commented Nov 8, 2017 • edited by mattklein123 Loading

mattklein123 commented Nov 8, 2017

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

mattklein123 commented Nov 20, 2017

pschultz commented Nov 20, 2017

lita commented Nov 20, 2017

pschultz commented Nov 20, 2017

mattklein123 commented Nov 20, 2017

lita commented Nov 26, 2017 • edited Loading

lita commented Nov 26, 2017

mattklein123 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

mattklein123 left a comment

Choose a reason for hiding this comment

lita commented Nov 8, 2017 •

edited by mattklein123

Loading

lita commented Nov 26, 2017 •

edited

Loading