stats: Create stats for http codes with the symbol table. #6733

jmarantz · 2019-04-27T19:36:21Z

Description: The HTTP response stats are dynamically generated in response to HTTP response codes from upstream, combined with cluster and vhost names. This changes makes that all occur using the SymbolTable API using a pattern that avoids global locks whenever an HTTP response code is re-used.

Note that there is even in a non-symbol table world, a global lock was always taken whenever a new http response stat had to be created, but there was none taken when a response was re-used. This brings the symbol-table API usage into that same state: no global locks for using a stat after it's been created.

Note also that as of now, the FakeSymbolTableImpl in use does not require any global locks at all, but this change is a pre-req for switching to the real SymbolTableImpl without taking a performance hit.
Risk Level: medium
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

…or stats. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…age objects, holding only one reference to the symbol table but maintaining RAII. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…tp::CodeStatsImpl. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…hink it doesn't add clarity or useful abstraction. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ructs. Signed-off-by: Joshua Marantz <jmarantz@google.com>

zuercher · 2019-04-30T16:18:59Z

@jmarantz holding off assigning this until it's a not a WIP any more

/wait

Signed-off-by: Joshua Marantz <jmarantz@google.com>

stale · 2019-05-07T19:52:01Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmarantz · 2019-05-07T19:55:22Z

/wait

I am actually homing in on getting this master-merged & working again.

…t-names for http responses. Not compiling yet, but pushing anyway. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

… it. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

ahedberg

Overall LGTM. Comments are minor.

source/extensions/filters/http/ext_authz/ext_authz.cc

source/extensions/filters/http/ratelimit/ratelimit.cc

test/extensions/filters/http/ext_authz/ext_authz_test.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz

Thanks @ahedberg @mattklein123 can you PTAL ?

test/extensions/filters/http/ext_authz/ext_authz_test.cc

source/extensions/filters/http/ext_authz/ext_authz.cc

source/extensions/filters/http/ratelimit/ratelimit.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

Thanks this is very cool stuff. Meta comment: Do you think the risk here is low? It seems more like medium? Some of this is a little scary.

I have a bunch of random comments which I'm flushing out. I didn't look at tests yet. Thank you!

/wait

mattklein123 · 2019-05-17T16:30:52Z

source/common/router/router.cc

- alt_stat_prefix_ = std::string(request_alt_name->value().getStringView()) + ".";
+ // NOTE: converting this header value into a StatName requires taking a
+ // global symbol-table lock. Is this scenario common enough to raise
+ // concern? Or is this effectively a test hook?


This is not a test feature. People do use this in production, but I'm not sure how often it's actually used. I think it's probably fine but I would maybe put in a TODO about thinking about improving this in the future and maybe put a warning in the docs on the documentation for this header? WDYT?

Got it. Found the doc:

./docs/root/configuration/http_filters/router_filter.rst ./docs/root/configuration/http_conn_man/header_sanitizing.rst

Since we are still using FakeSymbolTable this will not take a lock yet, so this PR doesn't introduce a lock in the hot-path.

Maybe I should add a tls-cache for cases where we really need to compose new StatName from string in the hotpath. I'm thinking there may be a few cases like this, which would justify adding this capability, rather than having to explain lock overhead details in user doc. Since we are still using Fakes I'd prefer to do that in a separate PR.

WDYT?

Yeah I think let's just put in a TODO for now and not add any docs. I don't think very many people are using this feature and I doubt they would notice the lock overhead even if we added it.

With that said, although I'm pretty certain this change is going to make this change faster, would it be worth it to add a microbenchmark for this code and then we can see the perf change?

Actually I have a microbenchmark, and I've been running it on this in various forms: test/common/http/codes_speed_test.cc. This makes things faster with real symbol tables, but it's a wash with fake symbol tables (since that really just degenerates to the same string concatenations in the hot-path).

Added #7003 and made this note a TODO(#7003).

I have a draft PR which provides a mechanism to fix this, but I don't know if it's worth the complexity for this feature. #7008

My feeling is that I'll leave that in draft mode until I see if there's more need for this beyond the header-controlled stat-prefix.

source/common/http/codes.cc

source/common/http/codes.h

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…of them is instantiated. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

Thanks looks good modulo remaining comments. WDYT about having a microbenchmark here first per my other comment?

/wait

source/common/http/codes.h

source/common/http/codes.cc

jmarantz · 2019-05-19T20:33:33Z

source/common/router/router.cc

- alt_stat_prefix_ = std::string(request_alt_name->value().getStringView()) + ".";
+ // NOTE: converting this header value into a StatName requires taking a
+ // global symbol-table lock. Is this scenario common enough to raise
+ // concern? Or is this effectively a test hook?


Actually I have a microbenchmark, and I've been running it on this in various forms: test/common/http/codes_speed_test.cc. This makes things faster with real symbol tables, but it's a wash with fake symbol tables (since that really just degenerates to the same string concatenations in the hot-path).

…tus codes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

Thanks, LGTM. Few small comments and general I would love to constify all the StatName variables where possible.

/wait.

source/common/http/codes.cc

source/common/router/config_impl.h

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

Nice!

jmarantz · 2019-05-21T19:55:20Z

Thanks for the review of this deep complex PR @mattklein123

* master: (65 commits) proto: Add PATCH method to RequestMethod enum (envoyproxy#6737) exe: drop unused deps on zlib compressor code (envoyproxy#7022) coverage: fix some misc coverage (envoyproxy#7033) Enable proto schema for router_check_tool (envoyproxy#6992) stats: rework stat sink flushing to centralize counter latching (envoyproxy#6996) [test] convert lds api test config stubs to v2 (envoyproxy#7021) router: scoped rds (2c): implement scoped rds API (envoyproxy#6932) build: Add option for size-optimized binary (envoyproxy#6960) test: adding an integration test framework for file-based LDS (envoyproxy#6933) doc: update obsolete ref to api/XDS_PROTOCOL.md (envoyproxy#7002) dispatcher: faster runOnAllThreads (envoyproxy#7011) example: add csrf sandbox (envoyproxy#6805) fix syntax of gcov exclusion zone. (envoyproxy#7023) /runtime_modify: add support for query params in body (envoyproxy#6977) stats: Create stats for http codes with the symbol table. (envoyproxy#6733) health check: fix more fallout from inline deletion change (envoyproxy#6988) Max heap fix (envoyproxy#7016) Add support to unregister from lifecycle notifications (envoyproxy#6984) build spdy_core_alt_svc_wire_format (envoyproxy#7010) ext_authz: Make sure initiateCall only called once (envoyproxy#6949) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

jmarantz added 2 commits April 26, 2019 14:11

stats: Convert HTTP Codes implementation to use the SymbolTable API f…

559b0f8

…or stats. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add and use a new helper class to manage a colleciton of StatNameStor…

ef296bc

…age objects, holding only one reference to the symbol table but maintaining RAII. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~stats: Create stats for http codes with the symbol table.~~ WiP stats: Create stats for http codes with the symbol table. Apr 27, 2019

jmarantz added 5 commits April 27, 2019 20:14

Use StatNameManagedContainer to help manage symbol name storage in Ht…

a98a873

…tp::CodeStatsImpl. Signed-off-by: Joshua Marantz <jmarantz@google.com>

tweak naming

f4e3c68

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Another round of name tweaks.

ab809e9

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove the nickname for uint8_t -- upon looking at the code I don't t…

a4e94d0

…hink it doesn't add clarity or useful abstraction. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Plumb through the stats prefix as StatName through the RequestInfo st…

215ad30

…ructs. Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot added the waiting label Apr 30, 2019

jmarantz mentioned this pull request Apr 30, 2019

stats: Add a new class StatNamePool to abstract some StatName memory management. #6757

Merged

jmarantz added 3 commits May 1, 2019 17:54

partially populate the zone info as StatName; not done yet.

fb02984

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into http-codes-with-symtab

29d6b63

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into http-codes-with-symtab

4036dbb

Signed-off-by: Joshua Marantz <jmarantz@google.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 7, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 7, 2019

jmarantz mentioned this pull request May 8, 2019

http: minor perf fix, not copying strings #6861

Merged

jmarantz added 2 commits May 8, 2019 16:51

Partial plumbing of StatName into everywhere required to generate sta…

c203b86

…t-names for http responses. Not compiling yet, but pushing anyway. Signed-off-by: Joshua Marantz <jmarantz@google.com>

fromat

53bb040

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label May 8, 2019

jmarantz added 8 commits May 8, 2019 17:02

Merge branch 'master' into http-codes-with-symtab

cb89eb2

Signed-off-by: Joshua Marantz <jmarantz@google.com>

checkpoint

a5dbe5e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into http-codes-with-symtab

50d2b31

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Fix tests & format.

1efef35

Signed-off-by: Joshua Marantz <jmarantz@google.com>

cleanups

aae1cc9

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add more commentary around StatNamePool::addReturningStorage().

9f74955

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into http-codes-with-symtab

0d56592

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Privatize name() for a couple of classes to avoid redundantly storing…

5c30bb4

… it. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz removed their assignment May 13, 2019

Clean up comments.

5ccd4c0

Signed-off-by: Joshua Marantz <jmarantz@google.com>

ahedberg reviewed May 13, 2019

View reviewed changes

source/extensions/filters/http/ext_authz/ext_authz.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/ratelimit/ratelimit.cc Outdated Show resolved Hide resolved

test/extensions/filters/http/ext_authz/ext_authz_test.cc Outdated Show resolved Hide resolved

mattklein123 self-assigned this May 13, 2019

Address review comments.

be57c0a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented May 14, 2019

View reviewed changes

test/extensions/filters/http/ext_authz/ext_authz_test.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/ext_authz/ext_authz.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/ratelimit/ratelimit.cc Outdated Show resolved Hide resolved

jmarantz added 2 commits May 14, 2019 11:47

Merge branch 'master' into http-codes-with-symtab

7c34b09

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into http-codes-with-symtab

a231ab9

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 requested changes May 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 17, 2019

jmarantz added 4 commits May 17, 2019 16:18

Merge branch 'master' into http-codes-with-symtab

aba608c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove some temps and member variables that are not used.

7d1bf54

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Fold RequestCodeGroup nested class into CodeStats, now that only one …

7ceb01a

…of them is instantiated. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Address review comments.

cb58e6f

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label May 19, 2019

mattklein123 requested changes May 19, 2019

View reviewed changes

source/common/http/codes.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label May 19, 2019

jmarantz commented May 19, 2019

View reviewed changes

Add missing test coverage and clean up gropu handling for invalid sta…

266beee

…tus codes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label May 19, 2019

Merge branch 'master' into http-codes-with-symtab

900e12c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 requested changes May 21, 2019

View reviewed changes

mattklein123 added the waiting label May 21, 2019

Constified a few variables and moved a lambda to a method.

ffb1e4b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label May 21, 2019

mattklein123 approved these changes May 21, 2019

View reviewed changes

jmarantz merged commit f683e0a into envoyproxy:master May 21, 2019

jmarantz deleted the http-codes-with-symtab branch May 21, 2019 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stats: Create stats for http codes with the symbol table. #6733

stats: Create stats for http codes with the symbol table. #6733

jmarantz commented Apr 27, 2019 •

edited

Loading

zuercher commented Apr 30, 2019

stale bot commented May 7, 2019

jmarantz commented May 7, 2019

ahedberg left a comment

jmarantz left a comment

mattklein123 left a comment

mattklein123 May 17, 2019

jmarantz May 19, 2019

mattklein123 May 19, 2019

jmarantz May 19, 2019

jmarantz May 19, 2019

jmarantz May 20, 2019

mattklein123 left a comment

jmarantz May 19, 2019

mattklein123 left a comment

mattklein123 left a comment

jmarantz commented May 21, 2019

stats: Create stats for http codes with the symbol table. #6733

stats: Create stats for http codes with the symbol table. #6733

Conversation

jmarantz commented Apr 27, 2019 • edited Loading

zuercher commented Apr 30, 2019

stale bot commented May 7, 2019

jmarantz commented May 7, 2019

ahedberg left a comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 May 17, 2019

Choose a reason for hiding this comment

jmarantz May 19, 2019

Choose a reason for hiding this comment

mattklein123 May 19, 2019

Choose a reason for hiding this comment

jmarantz May 19, 2019

Choose a reason for hiding this comment

jmarantz May 19, 2019

Choose a reason for hiding this comment

jmarantz May 20, 2019

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

jmarantz May 19, 2019

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

jmarantz commented May 21, 2019

jmarantz commented Apr 27, 2019 •

edited

Loading