-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
runtime: simplifying absl-flags based implementation #20169
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
748805e
to
44d38d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! It'd be great if the PR description could have a bit more description of what's changing. But this is great!
@@ -87,7 +133,8 @@ bool runtimeFeatureEnabled(absl::string_view feature) { | |||
IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", feature)); | |||
return false; | |||
} | |||
return absl::GetFlag(*flag); | |||
// We validate in map creation that the flag is a boolean. | |||
return flag->TryGet<bool>().value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOC: What is the difference between TryGet().value() and GetFlag()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetFlag takes the absl flag and returns a commandlineflag. Here we don't have the absl flag we only have the string name, so tryget allows us to get a coommandlineflag from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH! I see. That makes sense. I think I got confused because link 138 didn't change so I assumed that the type of flag didn't change. I think this is an example of how auto can actually make code a bit less readable, but it's a tradeoff, for sure. For bonus points consider changing auto to absl::CommandLineFlag on line 138. But totally at your discretion.
void maybeSetRuntimeGuard(absl::string_view name, bool value) { | ||
auto* flag = RuntimeFeaturesDefaults::get().getFlag(name); | ||
if (flag == nullptr) { | ||
IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", name)); | ||
return; | ||
} | ||
absl::SetFlag(flag, value); | ||
std::string err; | ||
flag->ParseFrom(value ? "true" : "false", &err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. The old code looked simpler, but I suspect it was deficient in some way. Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code is cleaner using absl::Flags, but while you can get from string to CommandLineFlag there's no public registry from string to absl::Flags - the reflection only goes from string to CommandLineFlag, so I had to switch from the "I know my type" absl::Flag to "am I a boolean or not?" CommandLineFlag variant to get rid of the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Same point about auto confusing me. Same bonus points for changing auto to absl::CommandLineFlag :)
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
cc @envoyproxy/envoy-mobile-maintainers I think we've solved the reflection problems but ccing just in case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for the explanation. Two suggests to eliminate the use of auto which led to me misunderstanding the code. But you can take or leave those suggestions.
@@ -87,7 +133,8 @@ bool runtimeFeatureEnabled(absl::string_view feature) { | |||
IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", feature)); | |||
return false; | |||
} | |||
return absl::GetFlag(*flag); | |||
// We validate in map creation that the flag is a boolean. | |||
return flag->TryGet<bool>().value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH! I see. That makes sense. I think I got confused because link 138 didn't change so I assumed that the type of flag didn't change. I think this is an example of how auto can actually make code a bit less readable, but it's a tradeoff, for sure. For bonus points consider changing auto to absl::CommandLineFlag on line 138. But totally at your discretion.
void maybeSetRuntimeGuard(absl::string_view name, bool value) { | ||
auto* flag = RuntimeFeaturesDefaults::get().getFlag(name); | ||
if (flag == nullptr) { | ||
IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", name)); | ||
return; | ||
} | ||
absl::SetFlag(flag, value); | ||
std::string err; | ||
flag->ParseFrom(value ? "true" : "false", &err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Same point about auto confusing me. Same bonus points for changing auto to absl::CommandLineFlag :)
-> Snow for non-googler pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions but otherwise this seems good to me
} | ||
} | ||
|
||
void RuntimeFeatures::restoreDefaults() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this stuff no longer necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it's replaced by the absl flagsaver infrastructure
// by runtime feature guards. If you add a guard of the form | ||
// RUNTIME_GUARD(envoy_reloadable_features_my_feature_name) | ||
// here you can guard code checking against "envoy.reloadable_features.my_feature_name". | ||
// Please note the swap of envoy_reloadable_features_ to envoy.reloadable_features.! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if people mess this up? Will it error out? If not, can we do something to avoid this mismatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'll work just fine actually, it just won't have standard envoy guard naming.
if ((!absl::StartsWith(name, "envoy_reloadable_features_") && | ||
!absl::StartsWith(name, "envoy_restart_features_")) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I understand since I'm not that familiar with this code: why this filtering? How are flags accessed if not via this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this largely helps us avoid pulling in erroneous flags in google 3, where we may end up with non-envoy command line flags.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
moving from a statically constructed envoy-flag-name -> absl flag map to using reflection to construct an envoy-flag-name -> commandline flag map, to avoid having to add 2 lines per runtime guard. Risk Level: Medium Testing: existing tests cover Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
moving from a statically constructed envoy-flag-name -> absl flag map to using reflection to construct an envoy-flag-name -> commandline flag map, to avoid having to add 2 lines per runtime guard.
Risk Level: Medium
Testing: existing tests cover
Docs Changes: n/a
Release Notes: n/a