stats: Make an object for the FIlesystem (Filesystem::Instance) to hold the stats counters. by jmarantz · Pull Request #5031 · envoyproxy/envoy

jmarantz · 2018-11-14T04:03:25Z

Description: Currently, stats counters/gauges are looked up every time a FileImpl is created. This change factors that out, introducing a Filesystem::Instance to hold the stats objects.

This is important for SymbolTables because lookups from string-literals take a lock, and we don't want to take locks while processing requests.
Risk Level: low
Testing: //test/...
Docs Changes:
Release Notes:

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

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

jmarantz · 2018-11-14T14:14:22Z

I think this is failing CI because of an API change to RawConnectionDriver, which now requires a Stats::Store, and this requires changing echo2_integration_test.cc in https://github.com/envoyproxy/envoy-filter-example . I guess that to avoid breaking any build at any time I have to issue two small PRs first:

change RawConnectionDriver to take an optional stat_store object and ignore it
change echo2_integration_test.cc to pass in a Stat::Store which it can get from BaseIntegrationTest via test_server_.stats().

A variation on this is to have BaseIntegrationTest provide a method for building a RawConnectionDriverPtr -- so that it can provide any other context required in the future. That feels a little nicer because I think it means doing this two-repo-PR dance a little less in the future, should more context be required. My concern is that this might be going against the design goals for RawConnectionDriver.

@mattklein123 @htuch @alyssawilk WDYT?

mattklein123 · 2018-11-14T17:45:27Z

@jmarantz at a high level, we don't create files in any high performance path. Is it worth the code churn here? (I haven't actually reviewed yet but trying to understand if the overall change is worth it.)

htuch · 2018-11-14T17:58:36Z

On the envoy-filter-example front, I have an idea how to make this dance less insane:

We move envoy-filter-example into the main Envoy repo, using a local repository in the WORKSPACE to point the example at Envoy source.
We export to envoy-filter-example as a read-only mirror (like data-plane-api), and swizzle the WORKSPACE to one that uses a remote repository.

This preserves envoy-filter-example as a standalone example of integrating Envoy, while avoiding the multi-phase commit dance (and breakages) we hit regularly these days. WDYT?

mattklein123 · 2018-11-14T18:00:23Z

This preserves envoy-filter-example as a standalone example of integrating Envoy, while avoiding the multi-phase commit dance (and breakages) we hit regularly these days. WDYT?

This sounds awesome. We could also then test it as part of the main CI.

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

…tead make an IsolatedStatsStore. This should avoid the problems copmiling echo2_integration_test.cc in a separate repo. Signed-off-by: Joshua Marantz <jmarantz@google.com>

alyssawilk · 2018-11-14T20:24:33Z

@jmarantz at a high level, we don't create files in any high performance path. Is it worth the code churn here? (I haven't actually reviewed yet but trying to understand if the overall change is worth it.)

Agreed - while I'm all for lockless code where possible, I also don't think of file operations as generally being performance critical - once you're looking at disk you're already on a slow path. Where is this problematic?

jmarantz · 2018-11-14T20:43:02Z

It's possible this might not be performance critical in real examples, but I also have refactored away for the need to change the API for filter-examples, so I'm hopeful that asan/tsan CI will just pass now.

It might still not be worth the churn, but there's less churn now :)

The reason this crossed my radar is that envoy-perf/siege/siege.py (which I wrote) uses a static file for serving origin content for the load test. IMO it's a justifiable refactor to give the file-system some context.

mattklein123 · 2018-11-14T23:47:06Z

OK can you merge master and we can take a look and discuss?

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

htuch · 2018-11-15T19:43:45Z

This preserves envoy-filter-example as a standalone example of integrating Envoy, while avoiding the multi-phase commit dance (and breakages) we hit regularly these days. WDYT?

This sounds awesome. We could also then test it as part of the main CI.

I've created #5054 to track. I might have cycles to work on this coming up, so tentatively assigned to me.

jmarantz · 2018-11-15T19:54:22Z

Very cool, @htuch.

FWIW this PR is not blocked on that now, but on the merging of master, which just added many instantiations of Api::ApiImpl throughout tests, and this PR makes it impossible to default-construct an Api::ApiImpl. It's of course still possible (but more cumbersome) to instantiate Api::ApiImpl in lots of places in tests.

I proposed (off-list) that createThread() be delegated to a ThreadSystem class, owned by ApiImpl but separately injectable. Most of the Api::ApiImpl instantiations that were just added to tests can then just instantiate a ThreadSystem.

I think this scales better as Api grows to encompass more functionality, some of which may require context to be passed into the constructor. I discussed this f2f with @eziskind and will follow up further.

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

jmarantz · 2018-11-21T15:46:06Z

ptal; this is now merged and ready for review.

For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in #4980, ThreadSystem in #5055 and #5072 and FileSystem in #5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this. This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled. Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks. The new class, Test::Global, creates a potential conflict with tests that had declared using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur. Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited. Testing: //test/... Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

mattklein123 · 2018-11-24T17:53:47Z

/retest

repokitteh-read-only · 2018-11-24T17:53:49Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5031 (comment) was created by @mattklein123.

see: more, trace.

mattklein123

LGTM, small comments. Thank you!

/wait

source/common/filesystem/filesystem_impl.h

mattklein123 · 2018-11-24T18:11:01Z

source/common/stats/thread_local_store.cc

  ASSERT(scopes_.empty());
 }

+void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) {


This seems unrelated to filesystems. Thoughts on moving this part of the change to a different PR?

The reason I needed to tackle this was that pre-declaring the stats for the filesystem at API creation time made them all leak through the late application of the StatsMatcher and broke the StatsMatcher integration test, which already had one hardcoded exception in it.

I felt it better to clean up the matcher than had a lot more exceptions to the integration-test.

However what I can do is put that into its own PR which we can review first.

Great, SGTM. Thanks for breaking things apart.

Pulled into #5105 which can be reviewed without the file-system change.

#5105 is merged now and so is no longer part of this PR.

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

For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in envoyproxy#4980, ThreadSystem in envoyproxy#5055 and envoyproxy#5072 and FileSystem in envoyproxy#5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this. This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled. Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks. The new class, Test::Global, creates a potential conflict with tests that had declared using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur. Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited. Testing: //test/... Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

…ld the stats counters. (envoyproxy#5031) Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

jmarantz added 2 commits November 13, 2018 22:58

Make an object for the FIlesystem (Filesystem::Instance) and hold the…

70fdab2

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

add comments and use new rejects() layer.

e03c578

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

jmarantz added 3 commits November 14, 2018 14:06

hack out API from base integration infra for now.

5e9f0b2

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

Merge branch 'master' into filesystem-stats

503119f

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

Revert the RawConnectionDriver to avoid taking a Stats::Store and ins…

14918d0

…tead make an IsolatedStatsStore. This should avoid the problems copmiling echo2_integration_test.cc in a separate repo. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into filesystem-stats

0b95183

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

mattklein123 self-assigned this Nov 15, 2018

mattklein123 added the waiting label Nov 15, 2018

htuch mentioned this pull request Nov 15, 2018

Make envoyproxy/envoy the source-of-truth for envoy-filter-example #5054

Open

get some tests working.

820477f

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

jmarantz mentioned this pull request Nov 19, 2018

test: Add a mechanism for creating singletons in tests #5086

Merged

jmarantz added 3 commits November 21, 2018 09:37

Merge branch 'master' into filesystem-stats

968af4b

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

Merge branch 'master' into filesystem-stats

93f09ea

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

format

27cd0c1

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

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

all tests working.

389561e

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

Merge branch 'master' into filesystem-stats

74350b4

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

mattklein123 requested changes Nov 24, 2018

View reviewed changes

repokitteh-read-only bot added waiting labels Nov 24, 2018

jmarantz added 2 commits November 24, 2018 13:42

Merge branch 'master' into filesystem-stats

b73e444

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

declare the default timeout as const.

c8a77de

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

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

mattklein123 added the waiting label Nov 24, 2018

Merge branch 'master' into filesystem-stats

4913ae9

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

jmarantz mentioned this pull request Nov 24, 2018

stats: Filter out rejected stats from scope caches when the stat-matcher is applied. #5105

Merged

Merge branch 'master' into filesystem-stats

aa8d531

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

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

mattklein123 approved these changes Nov 26, 2018

View reviewed changes

mattklein123 merged commit a59709c into envoyproxy:master Nov 26, 2018

jmarantz deleted the filesystem-stats branch November 26, 2018 21:38

Conversation

jmarantz commented Nov 14, 2018

Uh oh!

jmarantz commented Nov 14, 2018

Uh oh!

mattklein123 commented Nov 14, 2018

Uh oh!

htuch commented Nov 14, 2018

Uh oh!

mattklein123 commented Nov 14, 2018

Uh oh!

alyssawilk commented Nov 14, 2018

Uh oh!

jmarantz commented Nov 14, 2018

Uh oh!

mattklein123 commented Nov 14, 2018

Uh oh!

htuch commented Nov 15, 2018

Uh oh!

jmarantz commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Nov 21, 2018

Uh oh!

mattklein123 commented Nov 24, 2018

Uh oh!

repokitteh-read-only bot commented Nov 24, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 Nov 24, 2018

Choose a reason for hiding this comment

Uh oh!

jmarantz Nov 24, 2018

Choose a reason for hiding this comment

Uh oh!

mattklein123 Nov 24, 2018

Choose a reason for hiding this comment

Uh oh!

jmarantz Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarantz commented Nov 15, 2018 •

edited

Loading

jmarantz Nov 24, 2018 •

edited

Loading