Skip to content

stats: use counterFromStatName for ratelimit filters#7573

Merged
jmarantz merged 12 commits intoenvoyproxy:masterfrom
jmarantz:thrift-ratelimit-stats
Jul 23, 2019
Merged

stats: use counterFromStatName for ratelimit filters#7573
jmarantz merged 12 commits intoenvoyproxy:masterfrom
jmarantz:thrift-ratelimit-stats

Conversation

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Jul 14, 2019

Description: Another in a series of PRs to save StatNames at construction time, for lock-free-in-hot-path creation of scoped stats at request-time. This one captures the rate-limiting related stats, which occur in two different filters.

This PR also adds a formatting check to avoid direct calls to create counters, gauges, and histograms by name. Because there are still some remaining offenders, a whitelist is added with comments/TODOs to remove from this list until they are gone.

This is another step toward resolving #4196 and enabling submission of #4980.
Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

jmarantz added 9 commits June 12, 2019 15:32
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
… in the codebase.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz changed the title WiP stats: use counterFromStatName for ratelimit filters stats: use counterFromStatName for ratelimit filters Jul 15, 2019
@jmarantz jmarantz marked this pull request as ready for review July 15, 2019 18:50
@jmarantz jmarantz requested a review from zuercher as a code owner July 15, 2019 18:50
@jmarantz
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

@mattklein123 mattklein123 self-assigned this Jul 16, 2019
Signed-off-by: Joshua Marantz <jmarantz@google.com>
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 with 2 questions.

/wait-any

const bool failure_mode_deny_;
const absl::optional<Grpc::Status::GrpcStatus> rate_limited_grpc_status_;
Http::Context& http_context_;
Stats::StatNamePool stat_name_pool_;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not just embed this inside the StatNames structure on the next line? It seems like that would be less magical?

echo "Installing requirements..."
pip3 install --upgrade pip
pip3 install -r requirements.txt
pip3 -q install --upgrade pip
Copy link
Member

Choose a reason for hiding this comment

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

?

jmarantz added 2 commits July 22, 2019 16:10
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
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.

Nice!

@jmarantz jmarantz merged commit 4405d68 into envoyproxy:master Jul 23, 2019
@jmarantz jmarantz deleted the thrift-ratelimit-stats branch July 23, 2019 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants