added json output support for stats handler #1865

ramaraochavali · 2017-10-16T15:58:44Z

ramaraochavali · 2017-10-16T16:01:49Z

source/server/http/admin.cc

+      response.add(fmt::format("{}: {}\n", stat.first, stat.second));
+    }
+  } else {
+    std::string format = params.begin()->first;


Tried to use Field, FieldPtr and create json but not able to include them here.

Yeah, thanks for trying. See TODO request above.

mattklein123 · 2017-10-16T16:57:47Z

@ramaraochavali before I dive into full review, a few things:

Will need DCO (see contributing guide).
Please get the PR compiling/passing tests.
Will need docs here: https://envoyproxy.github.io/envoy/operations/admin.html
Will need a test here https://github.com/envoyproxy/envoy/blob/master/test/integration/integration_admin_test.cc that the body returned is actually valid JSON.

ramaraochavali · 2017-10-17T10:59:58Z

@mattklein123 I have updated the doc and added an integration test that validates the json. I have signed the DCO in my last commit. But still it shows as failed? Do I have to commit with a new branch?
All tests are passing. Can you please review now?

mattklein123 · 2017-10-17T15:19:22Z

@ramaraochavali every commit has to have DCO. Please just squash/rebase and force push the entire PR on current master (to fix OSX tests) and then we can review. Thank you.

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2017-10-17T16:35:19Z

@mattklein123 done..did a force commit as single one

mattklein123

Looks good, thanks for the contribution. Few small comments.

mattklein123 · 2017-10-17T18:11:00Z

docs/operations/admin.rst

+
+  .. http:get:: /stats?format=json
+
+    Same as /stats but outputs them in json format. This can be used for programmatic access of stats.


nit: "Same as /stats the output is in JSON format"

mattklein123 · 2017-10-17T18:12:05Z

source/server/http/admin.cc

@@ -31,8 +31,17 @@
 #include "common/upstream/host_utility.h"

 #include "fmt/format.h"
+#include "rapidjson/document.h"


I would prefer that we don't like rapidjson out into the rest of the code, but it's OK for now. Can you please add above the includes:

"TODO(mattklein123): Switch to JSON interface methods and remove rapidjson dependency"

mattklein123 · 2017-10-17T18:12:30Z

source/server/http/admin.cc

+      response.add(fmt::format("{}: {}\n", stat.first, stat.second));
+    }
+  } else {
+    std::string format = params.begin()->first;


Yeah, thanks for trying. See TODO request above.

mattklein123 · 2017-10-17T18:13:05Z

source/server/http/admin.cc

-  for (auto stat : all_stats) {
-    response.add(fmt::format("{}: {}\n", stat.first, stat.second));
+  if (params.size() == 0) {
+    // No Arguments so use the standard


nit: "No arguments so use the standard output." (period end of sentence)

mattklein123 · 2017-10-17T18:15:17Z

source/server/http/admin.cc

+    }
+  } else {
+    std::string format = params.begin()->first;
+    std::string formatvalue = params.begin()->second;


Please use snake case for all vars (e.g., "format_value", "stats_array" below, etc.). Won't comment on each one.

will change.. this habit from my lot of java coding :-)

mattklein123 · 2017-10-17T18:16:09Z

source/server/http/admin.cc

+    std::string format = params.begin()->first;
+    std::string formatvalue = params.begin()->second;
+    if (format == "format" && formatvalue == "json") {
+      rapidjson::Document document;


nit: Can we move the actual JSON creation into a helper function. We will likely have other output types in the future.

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2017-10-18T05:45:33Z

@mattklein123 Thanks for your quick review. Addressed all the comments. Should be good to go. Let me know if further changes are required

mattklein123

Thanks, looks good. Few tiny nits then ready to go.

mattklein123 · 2017-10-18T16:34:06Z

docs/operations/admin.rst

+
+  .. http:get:: /stats?format=json
+
+  Same as /stats the output is in JSON format. This can be used for programmatic access of stats.


Please switch first sentence to "Outputs /stats in JSON format."

mattklein123 · 2017-10-18T16:34:27Z

source/server/http/admin.cc

  // We currently don't support timers locally (only via statsd) so just group all the counters
  // and gauges together, alpha sort them, and spit them out.
+  Http::Code rc = Http::Code::OK;
+  Http::Utility::QueryParams params = Http::Utility::parseQueryString(url);


mattklein123 · 2017-10-18T16:34:44Z

source/server/http/admin.cc

+      response.add(fmt::format("{}: {}\n", stat.first, stat.second));
+    }
+  } else {
+    std::string format_key = params.begin()->first;


nit: const for this var and the the following

mattklein123 · 2017-10-18T16:35:00Z

source/server/http/admin.cc

+    std::string format_key = params.begin()->first;
+    std::string format_value = params.begin()->second;
+    if (format_key == "format" && format_value == "json") {
+      std::string json_stats = statsAsJson(all_stats);


nit: don't need local var, just fold function call into the following line.

mattklein123 · 2017-10-18T16:35:24Z

source/server/http/admin.h

@@ -115,7 +115,7 @@ class AdminImpl : public Admin,
  void addOutlierInfo(const std::string& cluster_name,
                      const Upstream::Outlier::Detector* outlier_detector,
                      Buffer::Instance& response);
-
+  std::string statsAsJson(std::map<std::string, uint64_t> all_stats);


nit: newline after this line.

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2017-10-19T03:27:44Z

@mattklein123 I think have addressed all comments. Have a final look once. Sorry because of lack of my recent c++ coding exp, took more time for you to review I guess. :-)

mattklein123

Thanks! Looks good.

mattklein123 · 2017-10-19T04:09:17Z

source/server/http/admin.h

@@ -115,6 +115,7 @@ class AdminImpl : public Admin,
  void addOutlierInfo(const std::string& cluster_name,
                      const Upstream::Outlier::Detector* outlier_detector,
                      Buffer::Instance& response);
+  std::string statsAsJson(std::map<std::string, uint64_t> all_stats);


oops sorry one more thing, can you make this const std::map<std::string, uint64_t>&. Sorry! :)

No issues. committed..please check

one more thing

Signed-off-by: Rama <[email protected]>

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

ramaraochavali mentioned this pull request Oct 16, 2017

Support additional output format in stats admin interface #1862

Closed

ramaraochavali commented Oct 16, 2017

View reviewed changes

force commit

698ead5

Signed-off-by: Rama <[email protected]>

ramaraochavali force-pushed the feature/stats-json branch from 48d7ee0 to 698ead5 Compare October 17, 2017 16:23

mattklein123 reviewed Oct 17, 2017

View reviewed changes

ramaraochavali added 2 commits October 18, 2017 10:40

addressed review comments

7c6b725

Signed-off-by: Rama <[email protected]>

format correction

2fb248e

Signed-off-by: Rama <[email protected]>

mattklein123 reviewed Oct 18, 2017

View reviewed changes

ramaraochavali added 2 commits October 19, 2017 08:34

added minor review comments

141da18

Signed-off-by: Rama <[email protected]>

format admin.h

98ea777

Signed-off-by: Rama <[email protected]>

mattklein123 previously approved these changes Oct 19, 2017

View reviewed changes

mattklein123 reviewed Oct 19, 2017

View reviewed changes

minor review comments

e723c3e

Signed-off-by: Rama <[email protected]>

mattklein123 approved these changes Oct 19, 2017

View reviewed changes

mattklein123 merged commit d5695aa into envoyproxy:master Oct 19, 2017

ramaraochavali deleted the feature/stats-json branch October 20, 2017 04:12

jpsim pushed a commit that referenced this pull request Nov 28, 2022

tests: fix event tracker integration tests (#1865)

a854e1b

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

tests: fix event tracker integration tests (#1865)

6fa0459

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added json output support for stats handler #1865

added json output support for stats handler #1865

ramaraochavali commented Oct 16, 2017 •

edited

Loading

ramaraochavali Oct 16, 2017

mattklein123 Oct 17, 2017

mattklein123 commented Oct 16, 2017

ramaraochavali commented Oct 17, 2017

mattklein123 commented Oct 17, 2017

ramaraochavali commented Oct 17, 2017

mattklein123 left a comment

mattklein123 Oct 17, 2017

mattklein123 Oct 17, 2017

mattklein123 Oct 17, 2017

mattklein123 Oct 17, 2017

mattklein123 Oct 17, 2017

ramaraochavali Oct 18, 2017

mattklein123 Oct 17, 2017

ramaraochavali Oct 18, 2017

ramaraochavali commented Oct 18, 2017

mattklein123 left a comment

mattklein123 Oct 18, 2017

mattklein123 Oct 18, 2017

mattklein123 Oct 18, 2017

mattklein123 Oct 18, 2017

mattklein123 Oct 18, 2017

ramaraochavali commented Oct 19, 2017

mattklein123 left a comment

mattklein123 Oct 19, 2017

ramaraochavali Oct 19, 2017


		.. http:get:: /stats?format=json

		Same as /stats but outputs them in json format. This can be used for programmatic access of stats.


		.. http:get:: /stats?format=json

		Same as /stats the output is in JSON format. This can be used for programmatic access of stats.

added json output support for stats handler #1865

added json output support for stats handler #1865

Conversation

ramaraochavali commented Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Oct 16, 2017

ramaraochavali commented Oct 17, 2017

mattklein123 commented Oct 17, 2017

ramaraochavali commented Oct 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Oct 18, 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

ramaraochavali commented Oct 19, 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

ramaraochavali commented Oct 16, 2017 •

edited

Loading