grpc_stats filter: Add config options to restrict number of stats#10467
grpc_stats filter: Add config options to restrict number of stats#10467htuch merged 20 commits intoenvoyproxy:masterfrom
Conversation
Add options to either put all methods into the same per-cluster stat, or to whitelist which methods to create stats for. Additionally, start a deprecation process for the default mode being to create a stat for all methods, to remove an unsafe default. Fixes: envoyproxy#10445 Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
This is still a bit rough, but I wanted to get some feedback on:
|
|
cc @kyessenov |
| } | ||
|
|
||
| // Gets the stat prefix and underlying storage, depending on whether request_names is empty | ||
| std::pair<Stats::StatName, Stats::SymbolTable::StoragePtr> |
There was a problem hiding this comment.
you can just return the StoragePtr here...the StatName is trivially constructed on the StoragePtr.
Think of StoragePtr as std::string, and StatName as the absl::string_view referencing the same string.
There was a problem hiding this comment.
The problem is that in one case, the StoragePtr is allocated in this function, and in the other case, the StatName is retrieved from a member variable that is a StatName. So in one case, I am returning ownership, and in the other case I am not.
Given that requirement, can I simplify this somehow?
There was a problem hiding this comment.
I see, that makes sense. And in all cases we don't need a symbol-table lock to do this operation, which is nice.
I think I see an opportunity for a refactor, where we add a new class StatNameJoinPool which has a method:
StatName join(const std::vector<StatName>& parts);
that would simplify this call-site and also others. Basically every usage of SymbolTable::join() is kind of ugly.
I added #10516 to track this, but feel free to mark this resolved.
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
/wait |
|
@jmarantz Here's a version of the map that flattens the keys to a tuple<string, string>. The code-logic is simpler, this is probably faster in practice, but it involves more complicated c++isms. Thoughts? Reference: https://groups.google.com/forum/#!topic/abseil-io/jvNJAqTQDIA |
mattklein123
left a comment
There was a problem hiding this comment.
One quick language comment. The API LGTM, thanks.
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
can you ping when Ci is passing? Thanks! /wait |
| // Envoy, using unbounded memory and potentially slowing down stats pipelines. | ||
| // | ||
| // .. attention:: | ||
| // If neither `stats_allowlist` nor `stats_for_all_methods` is set, the behavior |
There was a problem hiding this comment.
Nit: individual_method_stats_allowlist
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
/retest |
|
🐴 hold your horses - no failures detected, yet. |
|
/azp run envoy-linux |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
just noting that the 'release' test failed with not sure if that's a flake or regression, but re-running now. |
|
/azp envoy-linux run |
|
Command 'envoy-linux' is not supported by Azure Pipelines. Supported commands
See additional documentation. |
jmarantz
left a comment
There was a problem hiding this comment.
nice. Thank you for doing this!
just a few nits.
| } | ||
|
|
||
| // Gets the stat prefix and underlying storage, depending on whether request_names is empty | ||
| std::pair<Stats::StatName, Stats::SymbolTable::StoragePtr> |
There was a problem hiding this comment.
I see, that makes sense. And in all cases we don't need a symbol-table lock to do this operation, which is nice.
I think I see an opportunity for a refactor, where we add a new class StatNameJoinPool which has a method:
StatName join(const std::vector<StatName>& parts);
that would simplify this call-site and also others. Basically every usage of SymbolTable::join() is kind of ugly.
I added #10516 to track this, but feel free to mark this resolved.
source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.h
Outdated
Show resolved
Hide resolved
|
@htuch Can you approve the api again? Or do you want to re-evaluate in light of the conversation about compatibility/defaults? |
htuch
left a comment
There was a problem hiding this comment.
Given that the current policy allows for WKT defaults to switch, let's land this one. We should continue that discussion in parallel, thanks.
| case envoy::extensions::filters::http::grpc_stats::v3::FilterConfig::kStatsForAllMethods: | ||
| stats_for_all_methods_ = | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(proto_config, stats_for_all_methods, | ||
| context.runtime().snapshot().deprecatedFeatureEnabled( |
There was a problem hiding this comment.
@ggreenway Quick drive be here: I'm pretty sure there is not going to print a warning in the default case, right? (It will inc the stat I think.) This is going to make it hard/impossible for operators to figure what they did wrong. If I'm right can we do a follow up to get a warning printed somehow? cc @zuercher @alyssawilk who have more experience with the deprecation warning code.
There was a problem hiding this comment.
Good catch; I'll sort out a fix.
That's unfortunate, but it can't be fixed at this point. Eventually this commit will be far enough back in the history that the tool won't look at it. Until then, I think you just have to |
Add options to either put all methods into the same per-cluster
stat, or to whitelist which methods to create stats for.
Additionally, start a deprecation process for the default mode
being to create a stat for all methods, to remove an unsafe
default.
Risk Level: Low
Testing: Unit tests added
Docs Changes: done
Release Notes: added
Fixes #10445
Deprecated: added