Skip to content

admin: add /stats filtering capability#4371

Merged
ggreenway merged 13 commits intoenvoyproxy:masterfrom
ambuc:admin-stats-filter
Sep 13, 2018
Merged

admin: add /stats filtering capability#4371
ggreenway merged 13 commits intoenvoyproxy:masterfrom
ambuc:admin-stats-filter

Conversation

@ambuc
Copy link
Copy Markdown
Contributor

@ambuc ambuc commented Sep 7, 2018

Description: Added /stats stat name filtering. You can now hit the endpoint /stats?filter=foo and get just counters/gauges/histograms which have "foo" in their names. Compatible with json and used-only modes. Comes from an idea discussed in #4361. This doesn't support regex, just relies on absl::StrContains() under the hood.
Risk Level: Low.
Testing: Added two tests to admin_test.cc. Now tests the full space of filtering by used-only and/or filtering by string.
Docs Changes: I edited admin.rst to reflect this.
Release Notes: N/A

Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
@ggreenway
Copy link
Copy Markdown
Member

While we're adding this, should we just make it a regex so it covers more use cases and we don't need to support multiple options later?

@ambuc
Copy link
Copy Markdown
Contributor Author

ambuc commented Sep 7, 2018

@ggreenway I'm not opposed to this at all -- but I welcome recommendations on how to a) do this "efficiently", and b) how to encode the usual complexity of a regex in a URL. Is this done elsewhere in Envoy?

@ggreenway
Copy link
Copy Markdown
Member

a) I'm assuming that really simple regex will be fast enough, but that may be incorrect
b) percent/url encoding, although that may be really painful for CLI, although it looks like curl will do this for you: https://unix.stackexchange.com/questions/86729/any-way-to-encode-the-url-in-curl-command

But if the performance is too poor, even for a simple regex like "cluster", we can leave this as-is, and add another query-param for filtertype=regex later, or something.

Signed-off-by: James Buckland <jbuckland@google.com>
@ambuc
Copy link
Copy Markdown
Contributor Author

ambuc commented Sep 10, 2018

@ggreenway You were right, this is pretty easy to implement. Changes performed in 280029d.

Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Comment thread test/server/http/admin_test.cc
Comment thread source/server/http/admin.cc Outdated
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
@ambuc
Copy link
Copy Markdown
Contributor Author

ambuc commented Sep 11, 2018

@ggreenway, requesting review on this PR (now that I've kicked CircleCI).

@ramaraochavali
Copy link
Copy Markdown
Contributor

Thanks. LGTM other than one minor function name change nit .

Signed-off-by: James Buckland <jbuckland@google.com>
@ambuc
Copy link
Copy Markdown
Contributor Author

ambuc commented Sep 12, 2018

@ramaraochavali I made the change in c020e2a, thanks!

@ramaraochavali
Copy link
Copy Markdown
Contributor

Thanks. LGTM

Comment thread source/server/http/admin.h Outdated
Comment thread source/server/http/admin.h Outdated
Signed-off-by: James Buckland <jbuckland@google.com>
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for doing the work to handle regex.

@ggreenway ggreenway merged commit 325d5b1 into envoyproxy:master Sep 13, 2018
@ambuc ambuc deleted the admin-stats-filter branch September 13, 2018 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants