Skip to content

stats: Pre-allocate codec stats for http1 and http2#11135

Merged
jmarantz merged 35 commits intoenvoyproxy:masterfrom
jmarantz:prealloc-codec-stats
May 17, 2020
Merged

stats: Pre-allocate codec stats for http1 and http2#11135
jmarantz merged 35 commits intoenvoyproxy:masterfrom
jmarantz:prealloc-codec-stats

Conversation

@jmarantz
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz commented May 11, 2020 •
edited
Loading

Commit Message: Lazy-init codec stats as part of ClusterInfo and pass them into the codecs, rather than recreating the stats on every connection.
Additional Description:
Risk Level: medium
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Fixes: #11069, #8324

jmarantz added 13 commits May 9, 2020 22:24
@jmarantz
Factor out the atomic creation of an object to a template class.
1c7e41b 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
add unit test
f8612b1 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
factor out atomic/mutex combo logic for relatively contention-free al…
2bdf617 
…location of an object.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
back out codecs notes intended for a follow-up.
e9e80f2 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
h1 codec stats refactored; test/... in progress.
d9a04eb 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
h2 support (not quite done yet)
51603e2 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
add another check to the test.
8c899aa 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
use macros to factor out details of initializing codec stats.
497311a 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
format
5909bc0 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
add more tests.
f9d116b 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
grammar
e670dd2 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into atomic-object
c27103a 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into prealloc-codec-stats
edcc90e 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Copy Markdown
Contributor Author

jmarantz commented May 11, 2020

@danzh2010

jmarantz added 12 commits May 11, 2020 16:59
@jmarantz
temporarily comment out some tests to see if makes coverage not crash
181c8f3 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
use index-based looping to be sure we are nulling the array.
9077066 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
use memset for nulling, and a range-loop for deleting.
0f531fb 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
null-check prior to deleting ptr.
01e631e 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
use explict atomic refs in ctor/dtor.
4649355 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
use anon-namespace and rename the test-class.
c2fbc83 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
rename the 'ThreadTest' in lock_guard_test.cc too. Also back out acci…
66b6fa3 
…dental commits to coverage scripts.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
typos and clarification in doc.
3e5817a 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'atomic-object' into prealloc-codec-stats
0d2ec13 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into prealloc-codec-stats
8c032c5 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
cleanup
089a45b 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
lazy-init codec stats for admin and other usages as well.
18abb5f 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into prealloc-codec-stats
4c4790d 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz changed the title WiP stats: Pre-allocate codec stats stats: Pre-allocate codec stats for http1 and http2 May 12, 2020
@jmarantz jmarantz marked this pull request as ready for review May 12, 2020 22:26
jmarantz added 2 commits May 12, 2020 18:31
@jmarantz
remove commented notes.
b18d513 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
revert bazelversion
52035a3 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
mattklein123
mattklein123 requested changes May 13, 2020
View reviewed changes
Copy link
Copy Markdown
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.

This is really awesome. A couple of small comments. Also, this changes have a tendency to cause regressions in where the stats actually show up in the tree. Can you make sure some integration test actually verifies that they show up in the same place both for downstream and upstream? Maybe just add some checks to one of the protocol_integration_tests if they aren't already they? They can just be checks for stat existence IMO. Up to you. Thank you!

/wait

jmarantz added 2 commits May 13, 2020 17:36
@jmarantz
Merge branch 'master' into prealloc-codec-stats
ce83656 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Construct http* codec stats always via the atomic-ptr mechanism.
e001f47 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Copy Markdown
Contributor Author

jmarantz commented May 14, 2020

I'll look at adding some testing of the stat names; I don't see ton of testing for them in the integration tests.

I really wish there were less integration-test specific classes though. It's not super-obvious to me that the flow for creating stats in integration tests is faithful to the way it happens in prod.

Hold off on further review until I ping the next round, thanks.

/wait

jmarantz added 4 commits May 14, 2020 10:06
@jmarantz
Merge branch 'master' into prealloc-codec-stats
4712338 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into prealloc-codec-stats
1e8ce01 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
add testcases to ensure the scopes have not changed.
281c455 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
bazelversion back-out.
dd35e5c 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Copy Markdown
Contributor Author

jmarantz commented May 17, 2020

@mattklein123 ptal; I have added some stat-checks and verified that these checks also work on when patched into master.

I think this covers 3 of the 4 cases you suggested: dowsntream h1 and h2, and upstream h2. I could not find any cases in that test-suite where we were triggering h1 codec stats from upstream behavior. If you have an idea of where I might find one, I'd be happy to add it.

@jmarantz
add coverage for checking the existence of an h1 codec stat upstream.
0222b5c 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Copy Markdown
Contributor Author

jmarantz commented May 17, 2020

update: just after writing the previous comment I re-read yours and agree it should be sufficient to test for stat existence, so I covered the remaining quadrant with that easily. Thanks!

mattklein123
mattklein123 approved these changes May 17, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Awesome!

@jmarantz
Copy link
Copy Markdown
Contributor Author

jmarantz commented May 17, 2020

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented May 17, 2020

🔨 rebuilding ci/circleci: api (failed build)
🔨 rebuilding ci/circleci: docs (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #11135 (comment) was created by @jmarantz.

see: more, trace.

@jmarantz jmarantz merged commit c662775 into envoyproxy:master May 17, 2020
@jmarantz jmarantz deleted the prealloc-codec-stats branch May 17, 2020 22:34
spenceral added a commit to spenceral/envoy that referenced this pull request May 20, 2020
@spenceral
Merge branch 'master' into response-info-headers
ca19a15 
Signed-off-by: Spencer Lewis <slewis@squareup.com>

* master: (33 commits)
  docs: break release notes into categories (envoyproxy#11217)
  admin: extract more handlers to separate classes  (envoyproxy#11258)
  Load reporting service documentation  (envoyproxy#10962)
  http: testing 304-with-body behavior (envoyproxy#11261)
  fixing typos and breaking link issues (envoyproxy#11270)
  devex: initial commit of devcontainer setup (envoyproxy#11207)
  security: update policy for fix/disclosure SLOs. (envoyproxy#11243)
  http: fixing CONNECT to not advertise chunk encoding. (envoyproxy#11245)
  docs: update upstream network filters description (envoyproxy#11231)
  deps: update datadog tracer to v1.1.5 (envoyproxy#11253)
  test: Fix missing instantiation of parameterized tests. (envoyproxy#11247)
  fix go mirror when no changes (envoyproxy#11249)
  docs: host_rewrite -> host_rewrite_literal (envoyproxy#11229)
  wasm: update V8 to v8.3.110.9. (envoyproxy#11233)
  tls: update BoringSSL to 107c03cf (4103). (envoyproxy#11232)
  bazelci: always exclude nocoverage tag in coverage config (envoyproxy#11226)
  ci: save api revision in go-control-plane (envoyproxy#11220)
  build: fix cares build (envoyproxy#11225)
  stats: Pre-allocate codec stats for http1 and http2 (envoyproxy#11135)
  api: manifest based edge default documentation. (envoyproxy#11151)
  ...
asraa
asraa reviewed Jun 23, 2020
View reviewed changes
source/common/http/http1/codec_impl.h
* Wrapper struct for the HTTP/1 codec stats. @see stats_macros.h
*/
struct CodecStats {
using AtomicPtr = Thread::AtomicPtr<CodecStats, Thread::AtomicPtrAllocMode::DeleteOnDestruct>;
Copy link
Copy Markdown
Contributor

@asraa asraa Jun 23, 2020

Choose a reason for hiding this comment

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

Would it make sense if I moved these structs to a common codec_stats header? I want to share these structs between codecs and their forked legacy versions to avoid duplicating ClusterInfoImpl::http1CodecStats()

Copy link
Copy Markdown
Contributor Author

@jmarantz jmarantz Jun 23, 2020

Choose a reason for hiding this comment

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

Would be fine with me.

Note that the http3/quic ones may be a bit different (@danzh2010 ) because there are more of them and the master-list of them is maintained in an external codebase.

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

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

+1 more reviewer

@asraa asraa asraa left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

stats: name lookups occur on every new http1 connection may cause contention

3 participants

@jmarantz @asraa @mattklein123