Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stats: Capture all StatNames needed in the hystrix filter at construction time. #7046

Merged
merged 5 commits into from
May 28, 2019
Merged

stats: Capture all StatNames needed in the hystrix filter at construction time. #7046

merged 5 commits into from
May 28, 2019

Conversation

jmarantz
Copy link
Contributor

Description: To avoid taking locks in the hot-path, captures all the tokens needed to compose stat-names in the Hystrix sink when it's constructed. This avoids taking locks in the hot-path.

Also adds both real-symbol-table and fake-symbol-table variants to codes_speed_test.cc. On my machine this gets:

----------------------------------------------------------------------
Benchmark                            Time             CPU   Iterations
----------------------------------------------------------------------
BM_AddResponsesFakeSymtab         8284 ns         8284 ns        79715
BM_ResponseTimingFakeSymtab        597 ns          597 ns      1164865
BM_AddResponsesRealSymtab         4389 ns         4389 ns       163165
BM_ResponseTimingRealSymtab        262 ns          262 ns      2669827

Risk Level: low
Testing: hystrix_test and the speed test
Docs Changes: n/a
Release Notes: n/a

@jmarantz
Use StatName in hystrix implementation, and test both real and fake S…
6647f6e 
…ymbolTables in codes_speed_test.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
format
c71f10c 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz requested a review from ambuc May 22, 2019 21:37
ahedberg
ahedberg previously approved these changes May 23, 2019
View reviewed changes
ambuc
ambuc previously approved these changes May 23, 2019
View reviewed changes
Copy link
Contributor

@ambuc ambuc left a comment

Choose a reason for hiding this comment

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

LGTM, almost no comments. This is an encouraging PR demonstrating that we can move off string-based stat names!

test/common/http/codes_speed_test.cc Outdated Show resolved Hide resolved
@jmarantz
Merge branch 'master' into hystrix-with-stat-names
bb598c8 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Rename template calss name to be more spelled out.
e8090f6 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz dismissed stale reviews from ambuc and ahedberg via e8090f6 May 23, 2019 18:38
@jmarantz
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

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

see: more, trace.

@jmarantz
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

mattklein123
mattklein123 requested changes May 27, 2019
View reviewed changes
Copy link
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.

LGTM w/ small nit. Thank you.

/wait

source/extensions/stat_sinks/hystrix/hystrix.h Outdated Show resolved Hide resolved
@jmarantz
Declare StatName member variables as const.
de44859 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@mattklein123 mattklein123 merged commit 47a39f2 into envoyproxy:master May 28, 2019
@jmarantz jmarantz deleted the hystrix-with-stat-names branch May 28, 2019 13:20
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

@ahedberg ahedberg ahedberg left review comments

@ambuc ambuc ambuc left review comments

Assignees

@ahedberg ahedberg

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmarantz @ahedberg @mattklein123 @ambuc