Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass regexp filter to /stats/prometheus #7182

Merged
merged 2 commits into from
Jun 11, 2019
Merged

Pass regexp filter to /stats/prometheus #7182

merged 2 commits into from
Jun 11, 2019

Conversation

StrongMonkey
Copy link
Contributor

@StrongMonkey StrongMonkey commented Jun 5, 2019 •
edited
Loading

Description: Pass regexp parameter to /stats/prometheus handler
Risk Level: low
Issue: #7173

@StrongMonkey StrongMonkey force-pushed the add-regexp-promethues branch 6 times, most recently from 3c37e48 to cf72608 Compare June 5, 2019 19:16
@StrongMonkey
Copy link
Contributor Author

@mattklein123 Hi, Can you help review this pr?

@lizan lizan requested a review from snowp June 5, 2019 21:51
snowp
snowp suggested changes Jun 5, 2019
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Would you mind adding a test case that exercises the filtering?

source/server/http/admin.cc Outdated
@@ -753,8 +753,13 @@ Http::Code AdminImpl::handlerPrometheusStats(absl::string_view path_and_query, H
Buffer::Instance& response, AdminStream&) {
const Http::Utility::QueryParams params = Http::Utility::parseQueryString(path_and_query);
const bool used_only = params.find("usedonly") != params.end();
const absl::optional<std::regex> regex =
(params.find("filter") != params.end())
? absl::optional<std::regex>{std::regex(params.at("filter"))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably split this up into multiple lines for the sake of readability

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by, this is already computed above in handlerStats(). I would just pass that computation to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 I was actually looking at https://github.com/envoyproxy/envoy/blob/89a82de7a8368cd93b1255dfb3818f497330faf2/source/server/http/admin.cc#L1208 and seems like there is no good way unless we want to change the definition of MAKE_ADMIN_HANDLER. It is when the method is called from top-level api handler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, OK. Maybe just make a smaller helper method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper method added.

source/server/http/admin.h Outdated
@@ -437,6 +438,12 @@ class PrometheusStatsFormatter {
static bool shouldShowMetric(const std::shared_ptr<Stats::Metric>& metric, const bool used_only) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still used?

@StrongMonkey StrongMonkey force-pushed the add-regexp-promethues branch 6 times, most recently from dbe161f to 89a82de Compare June 6, 2019 22:37
@StrongMonkey
Copy link
Contributor Author

@snowp Hi, thanks for reviewing! I pushed my code to address your comments, and also added a unit test to exercise the regex for prometheus stats.

snowp
snowp suggested changes Jun 10, 2019
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment, otherwise this LGTM

source/server/http/admin.h Outdated
@@ -220,6 +220,11 @@ class AdminImpl : public Admin,
bool used_only,
const absl::optional<std::regex> regex = absl::nullopt,
bool pretty_print = false);
static absl::optional<std::regex> filterParam(Http::Utility::QueryParams params) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing but I'd probably put this in the anonymous namespace in the .cc file, no reason to put this on the class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Pass regexp filter to /stats/prometheus
86f9288 
Signed-off-by: Daishan Peng <daishan@rancher.com>
snowp
snowp previously approved these changes Jun 10, 2019
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

For the future: please avoid force pushing, individual commits make it easier to review changes made to the PR. We squash the PR before merging, so the extra commits don't matter in the end.

mattklein123
mattklein123 requested changes Jun 10, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than small nit. Thank you!

/wait

source/server/http/admin.h Outdated
@@ -413,7 +414,8 @@ class PrometheusStatsFormatter {
static uint64_t statsAsPrometheus(const std::vector<Stats::CounterSharedPtr>& counters,
const std::vector<Stats::GaugeSharedPtr>& gauges,
const std::vector<Stats::ParentHistogramSharedPtr>& histograms,
Buffer::Instance& response, const bool used_only);
Buffer::Instance& response, const bool used_only,
const absl::optional<std::regex> regex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const absl::optional<std::regex>&

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, updated!

pointer fix
110f935 
Signed-off-by: Daishan Peng <daishan@rancher.com>
mattklein123
mattklein123 approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mattklein123 mattklein123 merged commit 6ea29c7 into envoyproxy:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@snowp snowp snowp left review comments

Assignees

@snowp snowp

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@StrongMonkey @snowp @mattklein123