[admin] extract stats handlers to separate file#10750
[admin] extract stats handlers to separate file#10750mattklein123 merged 9 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
rulex123
changed the title
|
@rulex123 Please merge master to fix clang-tidy build problem. |
Signed-off-by: Erica Manno <erica.manno@gmail.com>
|
|
Signed-off-by: Erica Manno <erica.manno@gmail.com>
|
Sorry, can you do a master merge? I was going to do a quick check to see if I patched your source/... changes without your test/... changes do they pass. |
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
|
@jmarantz merged master |
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
This basically looks fine modulo the class-naming and a request to clean up a static regex while you're in here.
I mostly scanned through and I also patched this PR into a clean client, reverting the tests, to make sure the old tests work with new code (modulo a class-name change.
source/server/http/stats_handler.cc
Outdated
|
|
||
| const uint64_t RecentLookupsCapacity = 100; | ||
|
|
||
| const std::regex PromRegex("[^a-zA-Z0-9_]"); |
There was a problem hiding this comment.
no static non-pods
https://github.com/envoyproxy/envoy/blob/master/STYLE.md
The Google C++ style guide points out that non-PoD static and global variables are forbidden. This includes types such as std::string. We encourage the use of the advice in the C++ FAQ on the static initialization fiasco for how to best handle this.
also this should be in an anon-namespace or declared with static to reduce visibility, but you'll have to use the ref'd pattern to initialize.
I see this was in the old code too, but could fix while you are here?
source/server/http/stats_handler.h
Outdated
| namespace Envoy { | ||
| namespace Server { | ||
|
|
||
| class StatsHandlerImpl { |
There was a problem hiding this comment.
This is not implementing an interface, so I'd just call it StatsHandler.
Signed-off-by: Erica Manno <erica.manno@gmail.com>
Signed-off-by: Erica Manno <erica.manno@gmail.com>
|
@jmarantz addressed your feedback |
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
@envoyproxy/senior-maintainers
Signed-off-by: Erica Manno <erica.manno@gmail.com> Signed-off-by: pengg <pengg@google.com>
4 participants
Signed-off-by: Erica Manno erica.manno@gmail.com
Description: extract stats-related handlers from
admin.h|ccand into separate class (part of #5505 )Risk Level: low
Testing: pre-existing tests
Docs Changes: n/a
Release Notes: n/a