stats: envoy prometheus endpoint fails promlint #2597 #2757

pitiwari · 2018-03-08T01:15:50Z

Description: Fixes #2597"

The admin endpoint of envoy when queried as a prometheus endpoint fails promlint.
Promlint is a sanity check tool offered by Prometheus to check the correctness
of a prometheus endpoint.

following issues were fixed
-fixed issue with metric type being printed twice
-added unit test as well

Risk Level: Low

mrice32 · 2018-03-08T18:53:04Z

test/server/http/admin_test.cc

+  std::vector<Stats::Tag> tags1;
+  Stats::Tag tag2 = {"another_tag_name", "another_tag-value"};
+  tags1.push_back(tag2);
+  Stats::HeapRawStatDataAllocator alloc1_;


@pitiwari I think the issue is a little subtle, but I think this is because the allocators are being destructed before the stats themselves.

Fix: try declaring a single allocator as the first line in this test (to ensure it is the last thing to be destructed), and then use that allocator to allocate the data for all of the stats you create below.

Quick explanation of what's happening: the two lists counters and gauges are created before some of the allocators used to allocate the data for the CounterImpls and GuageImpls that they own. This will delay the destruction of these stats until after their allocators are destructed, which means that the CounterImpl and GaugeImpl destructors will try to call methods on an object that has already been destructed. The rule with these allocators is that they must outlive everything they allocate.

pitiwari · 2018-03-08T20:22:41Z

@mrice32 thanks. I had modified ut to create heap data allocator only once . UTS are passing now without needing any change in destructor, but still ref counting issue in HeapDataAllocator needs to be resolved since it is never incrementing or decrementing ref count.

mrice32 · 2018-03-08T21:25:56Z

@pitiwari, agreed. IIUC, that's being tracked #2453. Are you interested in tackling that in #2752? If so, that'd be awesome!

pitiwari · 2018-03-08T22:31:33Z

@mrice32 i can start looking into #2752 (#2453) . I have some other tasks in a plate so might take a week or two to fix it. In the meantime please review this pr and update with your comments

mrice32

Awesome stuff. Thanks for your work on this! Mostly test and docs nits.

mrice32 · 2018-03-09T17:31:16Z

test/server/http/admin_test.cc

@@ -293,5 +294,105 @@ TEST(PrometheusStatsFormatter, FormattedTags) {
  EXPECT_EQ(expected, actual);
 }

+TEST(PrometheusStatsFormatter, statsAsPrometheus) {


nit: can you give these tests more descriptive names so we know what each is testing? For example, something like NameCollision, etc?

mrice32 · 2018-03-09T17:32:22Z

test/server/http/admin_test.cc

+  EXPECT_EQ(2UL, PrometheusStatsFormatter::statsAsPrometheus(counters, gauges, response));
+}
+
+TEST(PrometheusStatsFormatter, statsAsPrometheus2) {


nit: statsAsPrometheus2 -> StatsAsPrometheus2 (same for the test above)

mrice32 · 2018-03-09T17:50:54Z

test/server/http/admin_test.cc

+  std::vector<Stats::Tag> tags;
+  Stats::Tag tag1 = {"a.tag-name", "a.tag-value"};
+  tags.push_back(tag1);
+  Stats::HeapRawStatDataAllocator alloc_;


nit: alloc_ -> alloc

mrice32 · 2018-03-09T17:56:17Z

test/server/http/admin_test.cc

+
+  Stats::TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)");
+  std::string name = "cluster.test_cluster_1.upstream_cx_total";
+  std::vector<Stats::Tag> tags;


nit: the var_name1 and var_name2 are a little confusing to read when they go all the way to 4. Also, they occasionally don't line up (tag2 is inserted into tags1). It might make things easier and more readable to add {} around each group to scope it so you can use the same variable names (without the numbers) without worrying about accessing the same data. The only items that need to be declared outside the scopes should be alloc, counters, and gauges. The other option would be to make a function for it.

mrice32 · 2018-03-09T18:03:00Z

test/server/http/admin_test.cc

+  // different tag names. In the end there only two metric names
+  // should be inserted, one for gauge and one for counter
+
+  Stats::TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)");


nit: are you using tag_extractor?

mrice32 · 2018-03-09T18:08:27Z

test/server/http/admin_test.cc

@@ -293,5 +294,105 @@ TEST(PrometheusStatsFormatter, FormattedTags) {
  EXPECT_EQ(expected, actual);
 }

+TEST(PrometheusStatsFormatter, statsAsPrometheus) {
+
+  // create two clusters and two gauges with metric name being same same and


nit: I would suggest that you change the comment to the following (similar for the comment in the second test):

Create two counters and two gauges with each pair having the same name, but having different tag names and values. `statsAsPrometheus()` should return two implying it found two unique stat names.

mrice32 · 2018-03-09T18:10:51Z

source/server/http/admin.h

-  static void statsAsPrometheus(const std::list<Stats::CounterSharedPtr>& counters,
-                                const std::list<Stats::GaugeSharedPtr>& gauges,
-                                Buffer::Instance& response);
+  static uint64_t statsAsPrometheus(const std::list<Stats::CounterSharedPtr>& counters,


nit: please update the method documentation with @return uint64_t explanation of return value.

mrice32 · 2018-03-09T18:14:08Z

test/server/http/admin_test.cc

+  // In the end there should be 4 metric names inserted 2 for gauge
+  //`and 2 for counter metric names
+
+  Stats::TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)");


nit: same as above - is this variable being used anywhere?

mrice32 · 2018-03-09T19:25:54Z

@pitiwari side note: if you add "Fixes #2597" in the description of the PR, it will automatically close the issue when it gets merged.

pitiwari · 2018-03-09T21:03:59Z

@mrice32 thanks i had updated the code based on review comments

mrice32 · 2018-03-09T21:44:58Z

@pitiwari Make sure you sign off on all your commits to allow the DCO to pass (see https://github.com/probot/dco#how-it-works). Also, make sure you run the formatter on your code. You can do this with docker by running ./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format'

mrice32

Thanks! A few more nits, then we should be good to go.

mrice32 · 2018-03-09T21:51:13Z

source/server/http/admin.h

@@ -248,11 +248,12 @@ class PrometheusStatsFormatter {
 public:
  /**
   * Extracts counters and gauges and relevant tags, appending them to
-   * the response buffer after sanitizing the metric / label names.
+   * the response buffer after sanitizing the metric / label names. Returns
+   * total number of metric types inserted in response


nit: please use @return doxygen syntax:

/** * Extracts counters and gauges and relevant tags, appending them to * the response buffer after sanitizing the metric / label names. * @return int64_t total number of metric types inserted in response. */

mrice32 · 2018-03-09T22:16:24Z

test/server/http/admin_test.cc

+
+TEST(PrometheusStatsFormatter, UniqueMetricName) {
+
+  //Create two counters and two gauges with each pair having the different name, 


nit - can we rephrase to: "Create two counters and two gauges, all with unique names. statsAsPrometheus() should return four implying it found four unique stat names."

done thanks

mrice32

LGTM, thanks for working through this PR. Awesome work! Also, please remember to add a signoff to all of your commits: https://github.com/probot/dco#how-it-works.

mrice32 · 2018-03-10T22:13:21Z

test/server/http/admin_test.cc

+
+  // Create two counters and two gauges with each pair having the same name,
+  // but having different tag names and values.
+  //`statsAsPrometheus()` should return two implying it found two unique stat names


Sorry, one more nit that I didn't notice the first few times through: please remove the backticks from statsAsPrometheus().

ggreenway

This looks great. Just need to DCO-sign all the commits and I think it's ready.

junr03 · 2018-03-12T16:47:01Z

looks like other than the DCO, @mrice32 is done with comments. @ggreenway or @htuch I think we can kick this up to you.

ggreenway · 2018-03-12T16:48:10Z

See this for how to fix DCO:

https://github.com/envoyproxy/envoy/blob/c5a982788e6b10d9a7831e201acc04d5953c703a/CONTRIBUTING.md#fixing-dco

pitiwari · 2018-03-12T19:38:46Z

rebased all the commits and added dco

ggreenway · 2018-03-12T19:42:30Z

It looks like your DCO value doesn't match the author value in your commits, so the DCO-bot still isn't accepting it. Sorry for the pedantic-ness of the DCO-bot. Can you please make the two match?

Description: The admin endpoint of envoy when queried as a prometheus endpoint fails promlint. Promlint is a sanity check tool offered by Prometheus to check the correctness of a prometheus endpoint. Signed-off-by: pitiwari <pitiwari@ebay.com>

pitiwari · 2018-03-12T23:38:00Z

seems like all tests passed, can we go ahead and commit ?

ggreenway · 2018-03-12T23:45:21Z

@pitiwari Thanks for making the DCO-bot happy. I'm going to let @htuch do a final review pass and merge.

htuch · 2018-03-13T18:02:44Z

source/server/http/admin.h

@@ -249,10 +249,11 @@ class PrometheusStatsFormatter {
  /**
   * Extracts counters and gauges and relevant tags, appending them to
   * the response buffer after sanitizing the metric / label names.
+   * @return int64_t total number of metric types inserted in response.


Nit: s/int64_t/uint64_t/

htuch · 2018-03-13T18:09:55Z

test/server/http/admin_test.cc

+    Stats::Tag tag = {"a.tag-name", "a.tag-value"};
+    cluster_tags.push_back(tag);
+    Stats::RawStatData* data = alloc.alloc(name);
+    Stats::CounterSharedPtr c(


c = std::make_shared<... (here and below)

htuch · 2018-03-13T18:12:19Z

test/server/http/admin_test.cc

+    Stats::RawStatData* data = alloc.alloc(name);
+    Stats::CounterSharedPtr c(
+        new Stats::CounterImpl(*data, alloc, std::string(name), std::move(cluster_tags)));
+    counters.push_back(c);


This scenario seems contrived. How does it happen in practice?

counterImpl/GuageImpl are derived classes from counter/gauge class so i create two counters and two gauges and pass them with same or different metric names in two different tests and check if metric name was same then gauge/counter are inserted only once for each unique metric name . Promlint tool was failing due to same metric name printed multiple times.

I understand how this happens in the test, I'm curious though how this happens in actual Envoy use. How did you discover this? What were the offending counters?

The envoy_listener_http_downstream_rq_xx counter appears many times, each time with a different set of tags and metrics that come with that tag (dimension).

For each upstream server you get 5 envoy_listener_http_downstream_rq_xx counters: one for each http responsecode (1, 2, 3, 4, 5) as the value for the "envoy_response_code_class" tag.

pitiwari · 2018-03-13T22:25:04Z

made changes based on review, please let me know if anything more needs to be changed after that i can squash all commits into single commit and push again

Signed-off-by: pitiwari <pitiwari@ebay.com>

pitiwari · 2018-03-14T19:34:03Z

@htuch can you please have a look. I had updated code based on your review comments

htuch

Thanks for the fix, this is great.

htuch requested a review from ggreenway March 8, 2018 18:20

htuch assigned ggreenway and htuch Mar 8, 2018

mrice32 mentioned this pull request Mar 8, 2018

stats: envoy prometheus endpoint fails promlint #2597 Destructor failing while calling derived class function #2722 #2752

Closed

mrice32 reviewed Mar 8, 2018

View reviewed changes

mrice32 reviewed Mar 9, 2018

View reviewed changes

mrice32 approved these changes Mar 10, 2018

View reviewed changes

ggreenway previously approved these changes Mar 12, 2018

View reviewed changes

pitiwari dismissed ggreenway’s stale review via 33877bb March 12, 2018 19:38

pitiwari force-pushed the prom_pr branch from c6a64f9 to 33877bb Compare March 12, 2018 19:38

pitiwari force-pushed the prom_pr branch from 33877bb to c77ff40 Compare March 12, 2018 20:58

pitiwari force-pushed the prom_pr branch from c77ff40 to dc8d067 Compare March 12, 2018 22:01

ggreenway previously approved these changes Mar 12, 2018

View reviewed changes

htuch suggested changes Mar 13, 2018

View reviewed changes

pitiwari dismissed ggreenway’s stale review via 2f87506 March 13, 2018 22:20

pitiwari force-pushed the prom_pr branch from 5f54666 to 6675369 Compare March 13, 2018 22:28

more changes based on review

eaf30b7

Signed-off-by: pitiwari <pitiwari@ebay.com>

pitiwari force-pushed the prom_pr branch from 6675369 to eaf30b7 Compare March 13, 2018 22:31

htuch approved these changes Mar 16, 2018

View reviewed changes

htuch merged commit 4d5c1a6 into envoyproxy:master Mar 16, 2018

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

add log entry fetch endpoint (envoyproxy#2757)

4b573fe


		TEST(PrometheusStatsFormatter, UniqueMetricName) {

		//Create two counters and two gauges with each pair having the different name,

stats: envoy prometheus endpoint fails promlint #2597 #2757

stats: envoy prometheus endpoint fails promlint #2597 #2757

Conversation

pitiwari commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

pitiwari commented Mar 8, 2018

mrice32 commented Mar 8, 2018

pitiwari commented Mar 8, 2018

mrice32 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

mrice32 commented Mar 9, 2018

pitiwari commented Mar 9, 2018

mrice32 commented Mar 9, 2018

mrice32 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

mrice32 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

junr03 commented Mar 12, 2018

ggreenway commented Mar 12, 2018

pitiwari commented Mar 12, 2018

ggreenway commented Mar 12, 2018

pitiwari commented Mar 12, 2018

ggreenway commented Mar 12, 2018

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

pitiwari commented Mar 13, 2018 • edited Loading

pitiwari commented Mar 14, 2018

htuch left a comment

Choose a reason for hiding this comment

pitiwari commented Mar 8, 2018 •

edited

Loading

mrice32 left a comment •

edited

Loading

pitiwari commented Mar 13, 2018 •

edited

Loading