stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system. #4997

jmarantz · 2018-11-08T17:47:56Z

Description: Currently, source/common/http/codes.cc provides static methods to compute stat names on every request and look them up. This refactor makes that a class with state, so that stat lookup strings can be partially precomputed and held in an object initialized at startup.

In the current state of the repo, this may have a small measurable benefit, but ultimately the stat-name strings need to be dynamically generated per-request because they incorporate scope, etc.

However, #4980 would benefit greatly from having this context plumbed in, because it could store pre-computed symbols for each of the tokens at startup -- that requires a lock. Combining multiple tokens together in the hot-path does not require a symbol-table lock, so this having context can help us avoid contention.

As a side-note, I feel like it's more work than I'd prefer to do this kind of plumbing. It would make it a lot easier if new global-ish objects could be held in some kind of factory and passed around, rather than passing around dozens of parameters to each constructor. Thoughts?

Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

… it in, and preconstruct strings. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…_view rather than std::string. Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 · 2018-11-08T18:13:25Z

@jmarantz this area has been on my perf-optimizatoin list for quite some time. Any chance of getting a baseline perf benchmark for this that we can use to gauge improvement?

As a side-note, I feel like it's more work than I'd prefer to do this kind of plumbing. It would make it a lot easier if new global-ish objects could be held in some kind of factory and passed around, rather than passing around dozens of parameters to each constructor. Thoughts?

I think you and I have the same opinions on statics, but IMO as long as we still pass the factory around and don't hold it as a static, this seems fine to me in general.

jmarantz · 2018-11-08T18:30:33Z

RE perf baseline: yes, will do and add in a separate PR, which can be a pre-req for this one. I think I would like to just do a microbenchmark that calls these recording functions, rather than having it at the system level with siege, where the results will be noisy and the marginally-more-optimal string operations I'm doing here may not matter compared to networking and moving payloads.

Note also that this still leaves the request-code-specific stat names. I'm thinking that, in the context of #4980, I would precompute symbols for some popular http codes (200, 204, 301, 302, 304, 400, 403, 404, 503 ?) and take a lock whenever any others were required. Thoughts? That can be discussed in that PR though, once this is merged.

RE passing factories: OK I'll look at doing that in a separate cleanup. Thanks!

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

mattklein123 · 2018-11-08T18:57:16Z

Note also that this still leaves the request-code-specific stat names. I'm thinking that, in the context of #4980, I would precompute symbols for some popular http codes (200, 204, 301, 302, 304, 400, 403, 404, 503 ?) and take a lock whenever any others were required. Thoughts? That can be discussed in that PR though, once this is merged.

Yeah, worth considering, once we have some baseline benchmarks.

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

jmarantz · 2018-11-08T19:11:31Z

Baseline speed test added in #4998 with these results:

---------------------------------------------------------
Benchmark                  Time           CPU Iterations
---------------------------------------------------------
BM_AddResponses         8220 ns       8220 ns      80376
BM_ResponseTiming        447 ns        447 ns    1575300

changes in this PR bring this to:

---------------------------------------------------------
Benchmark                  Time           CPU Iterations
---------------------------------------------------------
BM_AddResponses         5412 ns       5412 ns     120690
BM_ResponseTiming        354 ns        354 ns    1992006

These are all pretty low numbers -- we are talking 3 microseconds improvement per iteration (each iteration of AddResponses adds 9 responses). But it does show that there's some modest absolute improvement with this approach, and a significant percentage improvement.

Again, the goal here isn't necessarily to make this particular flow faster, it's to avoid it becoming slower in #4980.

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

jmarantz · 2018-11-11T17:02:54Z

I think this is ready for review -- I believe CI is a flake as I couldn't repro when locally running coverage tests.

mattklein123

Thanks, a high level comment to get started.

mattklein123 · 2018-11-14T00:28:37Z

include/envoy/server/filter_config.h

@@ -140,6 +141,12 @@ class FactoryContext {
   * @return OverloadManager& the overload manager for the server.
   */
  virtual OverloadManager& overloadManager() PURE;
+
+  /**
+   *


nit: extra newline

mattklein123 · 2018-11-14T00:37:10Z

include/envoy/server/instance.h

+  /**
+   * @return Http::CodeStats the http response-code stats
+   */
+  virtual Http::CodeStats& codeStats() PURE;


I'm not crazy about this being a server-wide thing and then having to be plumbed through everywhere. What's the reasoning for that? Can it potentially be per-HCM and then just exposed via HTTP filter callbacks? If you want it to be global can we create it in the singleton manager as part of the HCM (like the HTTP date provider)?

HCM==HttpConnectionManager I assume? How many HCMs are there? One per thread?

The reasoning for having it be global is that it's not going to be free to create or store, once SymbolTables are integrated. But if there were a bounded number created at startup it would be OK. There's no compelling reason there has to be only one, as long as they are not created on demand in the hot-path.

What are the drawbacks having them be plumbed everywhere? In my mind the most painful thing is the number of many edits that required. However, I'd be up for a refactor -- even in advance of this PR. One thought I had was making Server::Configuration::FactoryContext more broadly available, and that would make TimeSource, Http::CodeStats, SymbolTable, and whatever else was needed easier to get.

I hadn't really looked at HCM before but it looks like this is a way to pass around data that's derived from configuration, whereas Server::Configuration::FactoryContext looks more like it's sharing common data structures created by the server at startup, which feels like what this is.

What are the drawbacks having them be plumbed everywhere?

Because IMO it's an abstraction leakage. Envoy is not a dedicated HTTP server (though yes we always have the admin server) so it seems incorrect to create this thing and pass it around everywhere when it only applies to HTTP traffic.

IMO we can create one of these things per HCM (it's not per-thread, it's per listener effectively) or if we want one for all listeners you can create a singleton like here: https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/network/http_connection_manager/config.cc#L66

Fair enough -- individual HTTP-specific objects shouldn't be plumbed through where they are not needed. And using a singleton owned by a per-connection HTTP factory seems workable and I'll go with that for this PR. IIRC you told me once that pattern of singleton-management gets cleaned up between unit tests.

But it does seem like there's a few different http-specific objects that want to be once per server...what do you think of having a class to hold and/or create those (HttpContext or HttpFactory or similar), and have that passed into the HCM constructor? This would effectively be like the singletons but a little cleaner from a statics perspective. WDYT?

The singleton manager avoids statics, it's an actual object. Take a look at the implementation.

I don't think you're going to be much happier with the change I just made; I was trying to move stuff around for a cleaner dependency graph but I think it was better before I did this.

I just did a commit to see how it looks via the GH interface. I'll follow up further.

Per DM on slack I am adding a new Http::Context object to hold the httpTracer and Http::CodeStats.

The singleton-manager didn't work out due to same-thread assertions.

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

jmarantz · 2018-11-21T18:22:26Z

Note I still haven't addressed @mattklein123 's issue about having the http-codes be part of the server object, rather than being accessed only from http-specific objects, so this is not ready for review; I just wanted to merge master.

…er_impl.cc, though I think it probably doesn't belong there either. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

ramaraochavali · 2018-11-29T16:56:41Z

source/common/http/codes.cc

        .recordValue(info.response_time_.count());
  }
 }

+absl::string_view CodeStatsImpl::stripTrailingDot(absl::string_view str) {


WDYT about moving this to StringUtil with trailing char as argument?

I thought of that...but I'm not sure if this would be useful anywhere else, so I thought maybe it doesn't make that much sense to broaden the interface everyone has to look through.

If you think there might be another use-case I can broaden it.

And actually the only reason to have this function to to avoid removing all the cases of "prefix." throughout the code in this PR. I think instead I'll put a TODO to remove the function and the need for it.

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

jmarantz · 2018-11-30T02:40:19Z

/retest

repokitteh-read-only · 2018-11-30T02:40:22Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: mac (failed build)

🐱

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

see: more, trace.

mattklein123

Generally LGTM. A few small comments.

/wait

mattklein123 · 2018-12-01T22:32:21Z

source/common/http/codes.cc

 namespace Envoy {
 namespace Http {

-void CodeUtility::chargeBasicResponseStat(Stats::Scope& scope, const std::string& prefix,
-                                          Code response_code) {
+CodeStatsImpl::CodeStatsImpl() {}


nit: del empty constuctor/destructor

Based on past experience I think we can improve the compile/link times for Envoy if we have explicit non-inlined destructors for all virtual classes. But injecting them incrementally in a few PRs without broader evangelism isn't going to move the needle.

The Chromium style guide has similar observations:

https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-inlining-constructors-and-destructors

IMO the destructors are more important to leave outlined than the constructors, though, as I think the constructors will only cost you compile/link time when they are actually instantiated, whereas inlined destructors cost you compile-link speed every time they are referenced.

For now though I'll just inline them to match current style.

added #5182 for discussing inline dtors.

That's fine, I don't really care as long as we are generally consistent. We already do this for mocks for the same reason. I will stop commenting on this.

source/common/http/context_impl.cc

source/common/http/context_impl.h

mattklein123 · 2018-12-01T22:39:58Z

source/server/server.cc

-  Configuration::MainImpl* main_config = new Configuration::MainImpl();
-  config_.reset(main_config);
-  main_config->initialize(bootstrap_, *this, *cluster_manager_factory_);
+  config_.initialize(bootstrap_, *this, *cluster_manager_factory_);


I guess you decided to use the impl directly instead of the interface. Any particular reason for this? I guess it's an OK change. If we keep it can you change the comment up at line 313?

Edited comment. The reason I changed it was it seemed simpler to directly contain the ConfigImpl -- the two-phase construct process happens either way but it didn't seem important when the first phase occurred.

Let's discuss in the other PR.

mattklein123 · 2018-12-01T22:40:57Z

source/server/server.cc

@@ -493,8 +491,8 @@ void InstanceImpl::terminate() {
    flushStats();
  }

-  if (config_ != nullptr && config_->clusterManager() != nullptr) {
-    config_->clusterManager()->shutdown();
+  if (config_.clusterManager() != nullptr) {


I'm not sure how this can be a safe change if we checked for nullptr before. Are you sure that cluster manager exists at this point? This is related to my comment above, but is this change from interface to impl needed in this change?

config_ is now a direct object and not a unique_ptr so this is safe and IMO marginally simpler.

However you are right that this change is not critical to this PR and I am happy to back it out if you prefer.

actually I went ahead and backed it out. I'll issue it as a separate PR.

OK I remember why I messed with this now. clang-tidy doesn't like it the way the code is in the repo now. Because it grandfathers in old non-compliant code it was OK until I made edits near it, and my previous state (now reflected in #5184) makes it clang-tidy clean.

Also it's super-annoying to iterate with clang-tidy as it takes 40 minutes to run locally for me under docker, and it's non-deterministic in CI.

Here's the conundrum:

clang-tidy doesn't like naked calls to new, with Config* variables.

the semantics of the code as is require two pointers to the same memory: one owned
by Server that's got the interface type, and one local that has the Impl type. The Impl
type provides an initialize() method and the interface does not.

The implementation of the configure method requires that server->config_ be populated
Without a more significant refactoring I don't think it's possible to make this code work and also make clang-tidy happy.

Note that not much of this has anything to do with the Http::Context propagation or precalculation of StatsCode symbols, but I must've woken up clang-tidy by editing code nearby.

So I think there are a few choices here:

merge even though there are clang-tidy failures

do a more significant refactoring of the initialization() dance here

move forward with server: Contain the Configuration::MainImpl directly in server objects. #5184 despite @venilnoronha's concern that this adds another impl dependency to server.h

My vote is to move forward with #5814 -- I think it's the cleanest, most honest approach that doesn't require a significant refactor.

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

This was not germaine to this PR. This only was in the PR because during implementation at one point I was attempting to involve Http::Context more deeply in the ownership model for the tracer, but found that intractable. Along the way I had done this simplification -- which I still claim is incrementally better -- but is not essential for this PR. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

jmarantz · 2018-12-04T13:08:40Z

@mattklein123 ptal; I think this is ready for another round of review, now that #5184 is merged.

mattklein123

Nice

…ject, plumbed through the system. (envoyproxy#4997) * Make a class for Http::CodeUtility::chargeResponseTiming et al, plumb it in, and preconstruct strings. Add HttpContext:: to plumb through Http::CodeStats and HttpTracer and potentially other http-specific structures. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

jmarantz added 2 commits November 8, 2018 11:45

Make a class for Http::CodeUtility::chargeResponseTiming et al, plumb…

d70cb18

… it in, and preconstruct strings. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Use join() rather than fmt::format, and hold static strings in string…

9c49dec

…_view rather than std::string. Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 self-assigned this Nov 8, 2018

fix router test (empty prefix) and use const string_view for member v…

df7d7a1

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

jmarantz added 2 commits November 8, 2018 13:59

add speed-test.

da648b3

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

Remove libraries not needed by speed test.

ec66aaf

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

jmarantz mentioned this pull request Nov 8, 2018

stats: adds a speed-test for accumulating http response stats #4998

Merged

Merge branch 'master' into http-code-stats-as-object

d509154

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

mattklein123 requested changes Nov 14, 2018

View reviewed changes

mattklein123 added the waiting label Nov 14, 2018

jmarantz added 3 commits November 16, 2018 09:06

Merge branch 'master' into http-code-stats-as-object

cf7565d

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

Merge branch 'master' into http-code-stats-as-object

9cd564e

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

format

255c51b

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

repokitteh-read-only bot removed the waiting label Nov 21, 2018

mattklein123 added the waiting label Nov 22, 2018

Move the CodeStats instance out of the server and into listener_manag…

cca3bc9

…er_impl.cc, though I think it probably doesn't belong there either. Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label Nov 25, 2018

jmarantz changed the title ~~stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system.~~ WiP stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system. Nov 25, 2018

jmarantz added 2 commits November 26, 2018 14:40

Add HTTP Context object.

bdea1d7

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

Merge branch 'master' into http-code-stats-as-object

e7d59b2

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

mattklein123 added the waiting label Nov 27, 2018

jmarantz added 3 commits November 27, 2018 14:43

Merge branch 'master' into htp-context

60d7454

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

mostly working; need to commit to sync.

eb0be50

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

Merge branch 'master' into htp-context

60c4a73

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

ramaraochavali reviewed Nov 29, 2018

View reviewed changes

jmarantz added 2 commits November 29, 2018 13:31

Add comments to internal function descriptions.

aea8630

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

Merge branch 'master' into http-code-stats-as-object

30e0c15

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

mattklein123 requested changes Dec 1, 2018

View reviewed changes

repokitteh-read-only bot added waiting labels Dec 1, 2018

jmarantz added 3 commits December 2, 2018 09:00

Merge branch 'master' into http-code-stats-as-object

47e014e

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

Update comment and remove empty ctors/dtors from cc files.

21e5a34

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

Remove superfluous include.

3a2cef0

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

repokitteh-read-only bot removed the waiting label Dec 2, 2018

jmarantz added 4 commits December 2, 2018 11:30

Contain ConfigurationImpl directly in the servers, simplifying a few …

72e6dd3

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

Fix dereference.

522e235

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

all tests working.

01bd77a

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

jmarantz mentioned this pull request Dec 2, 2018

server: Contain the Configuration::MainImpl directly in server objects. #5184

Merged

jmarantz added 6 commits December 2, 2018 21:10

Merge branch 'master' into config-impl-contained

7802653

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

Reword comment.

c3508fa

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

Merge branch 'config-impl-contained' into http-code-stats-as-object

0c8ac51

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

Merge branch 'master' into http-code-stats-as-object

4b32611

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

Remove commented-out code.

4f0ebb5

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

attempt to fix clang-tidy errors.

d08fc60

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

mattklein123 approved these changes Dec 4, 2018

View reviewed changes

jmarantz merged commit 43fc779 into envoyproxy:master Dec 5, 2018

jmarantz deleted the http-code-stats-as-object branch December 5, 2018 01:53

jmarantz mentioned this pull request Dec 8, 2018

Init Http::ContextImpl::tracer_ when tracing is enabled, and test it. #5249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system. #4997

stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system. #4997

jmarantz commented Nov 8, 2018

mattklein123 commented Nov 8, 2018

jmarantz commented Nov 8, 2018

mattklein123 commented Nov 8, 2018

jmarantz commented Nov 8, 2018

jmarantz commented Nov 11, 2018

mattklein123 left a comment

mattklein123 Nov 14, 2018

mattklein123 Nov 14, 2018

jmarantz Nov 14, 2018

mattklein123 Nov 14, 2018

jmarantz Nov 14, 2018

mattklein123 Nov 14, 2018

jmarantz Nov 25, 2018 •

edited

Loading

jmarantz Nov 28, 2018 •

edited

Loading

jmarantz commented Nov 21, 2018

ramaraochavali Nov 29, 2018

jmarantz Nov 29, 2018

jmarantz commented Nov 30, 2018

repokitteh-read-only bot commented Nov 30, 2018

mattklein123 left a comment

mattklein123 Dec 1, 2018

jmarantz Dec 2, 2018

jmarantz Dec 2, 2018

mattklein123 Dec 2, 2018

mattklein123 Dec 1, 2018

jmarantz Dec 2, 2018

mattklein123 Dec 2, 2018

mattklein123 Dec 1, 2018

jmarantz Dec 2, 2018

jmarantz Dec 2, 2018

jmarantz Dec 2, 2018 •

edited

Loading

jmarantz commented Dec 4, 2018

mattklein123 left a comment

stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system. #4997

stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system. #4997

Conversation

jmarantz commented Nov 8, 2018

mattklein123 commented Nov 8, 2018

jmarantz commented Nov 8, 2018

mattklein123 commented Nov 8, 2018

jmarantz commented Nov 8, 2018

jmarantz commented Nov 11, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz Nov 25, 2018 • edited Loading

Choose a reason for hiding this comment

jmarantz Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

jmarantz commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Nov 30, 2018

repokitteh-read-only bot commented Nov 30, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz Dec 2, 2018 • edited Loading

Choose a reason for hiding this comment

jmarantz commented Dec 4, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

jmarantz Nov 25, 2018 •

edited

Loading

jmarantz Nov 28, 2018 •

edited

Loading

jmarantz Dec 2, 2018 •

edited

Loading