-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add command to dump dynamic config values (#421) #8676
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
base: main
Are you sure you want to change the base?
Add command to dump dynamic config values (#421) #8676
Conversation
dnr
left a comment
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.
please regenerate with the correct versions of protoc/protoc-gen-go to avoid the unrelated generated file changes
common/dynamicconfig/collection.go
Outdated
| } | ||
| } | ||
|
|
||
| func (c *Collection) GetClient() Client { |
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 used anywhere? I'd prefer not exposing the underlying Client if we don't really need to
| return err | ||
| } | ||
| func (s {{$P.Name}}TypedConstrainedDefaultSetting[T]) DefaultValue() any { | ||
| return s.cdef |
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 feels weird, s.cdef is a []TypedConstrainedValue[T], which is a type that's going to be pretty hard/awkward for any client code to use. I suppose you're only using it serialize to json or something, so it doesn't really matter what the type is?
| hasDefaultConstraint = true | ||
| } | ||
| } | ||
| // add default value if not present |
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 feels kind of weird. why do we need to insert the default values here? first of all, it doesn't work for settings with multiple constrained defaults. second, it's using more memory for something we don't actually need for looking up settings. why can't the code to retrieve settings just have logic to get the default from the registry instead of doing it here? it needs that anyway to get the default for a setting that's not preset in the file at all, right?
common/dynamicconfig/registry.go
Outdated
| globalRegistry registry | ||
| ) | ||
|
|
||
| func GetDefaultValueForKey(k Key) ConstrainedValue { |
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.
as above, this just doesn't make any sense for settings with constrained default values, there is no single default.
| - value: true | ||
| history.hostLevelCacheMaxSize: | ||
| - value: 8192 | ||
| - value: 8000 |
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.
why is this changed?
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.
My bad, I did this to test my changes and forgot revert it.
|
|
||
| message ConstrainedValue { | ||
| Constraints constraints = 1; | ||
| google.protobuf.Struct value = 2; |
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.
hmm, shouldn't it be a google.protobuf.Value instead? most configs are scalars, there are structs and lists but they're not the majority.
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.
ok
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.
I'm not sure it should be Value, should we just dump the struct as JSON? It could be simpler. But Value would be better than Struct, agree.
service/frontend/admin_handler.go
Outdated
| "go.temporal.io/server/api/adminservice/v1" | ||
| clusterspb "go.temporal.io/server/api/cluster/v1" | ||
| commonspb "go.temporal.io/server/api/common/v1" | ||
| dc "go.temporal.io/server/api/dynamicconfig/v1" |
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.
import should be named dynamicconfigspb ("spb" for "server proto buf")
service/frontend/admin_handler.go
Outdated
| } | ||
|
|
||
| protoValues := make([]*dc.ConstrainedValue, 0) | ||
| for _, ConstrainedValue := range dcEntry { |
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.
use lowercase for variable names
| for _, ConstrainedValue := range dcEntry { | |
| for _, cv := range dcEntry { |
service/frontend/admin_handler.go
Outdated
| dcEntry = append(dcEntry, defaultValue) | ||
| } | ||
|
|
||
| protoValues := make([]*dc.ConstrainedValue, 0) |
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.
| protoValues := make([]*dc.ConstrainedValue, 0) | |
| protoValues := make([]*dc.ConstrainedValue, 0, len(dcEntry)) |
or assign to specific indexes. or use util.MapSlice.
| } | ||
|
|
||
| message GetDynamicConfigurationsResponse { | ||
| repeated HostConfig host_config = 1; |
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.
why repeated? do we allow querying multiple hosts at once?
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.
Not now, but do have plan to make so.
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 was my idea, so you can compare dynamic configs across hosts. This will be a future capability though.
|
Thanks for the review @dnr I'm considering two alternatives.
Which approach would you prefer? I'm leaning toward first one as it's simpler and provides value for operators troubleshooting config issues. |
abb41c8 to
509df74
Compare
| return globalRegistry.settings[strings.ToLower(k.String())] | ||
| } | ||
|
|
||
| func GetDefaultValues(k Key) []ConstrainedValue { |
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.
| func GetDefaultValues(k Key) []ConstrainedValue { | |
| // DefaultValue returns the default value defined for the setting of the given key. | |
| func DefaultValue(k Key) []ConstrainedValue { |
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.
But I'm not even convinced we want this.
|
|
||
| message ConstrainedValue { | ||
| Constraints constraints = 1; | ||
| google.protobuf.Value value = 2; |
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.
I wonder if it would be simpler to give back a serialized JSON object here.
|
|
||
| // get global default value | ||
| if dcEntry == nil { | ||
| dcEntry = dynamicconfig.GetDefaultValues(k) |
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.
Do we even want to give back the default value? I was thinking we would return the "raw" values without deserializing them. I'm not sure if the default values are always JSON serializable.
I can look into this a bit more but @dnr can speak to it better than I can.
|
|
||
| protoValues := make([]*dynamicconfigspb.ConstrainedValue, 0, len(dcEntry)) | ||
| for _, cv := range dcEntry { | ||
| s, err := structpb.NewValue(normalizeValue(cv.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.
This isn't ideal, we should be able to return all of the values back. Users of this API will be surprised to not get responses back.
Reading the implementation of NewValue I'm not convinced that it will be able to convert everything that we support, which is why I think returning a JSON serialized representation is probably a better option.
We should also test this API with various complex configs to see that we return what is expected.
| string namespace = 1; | ||
| string namespace_id = 2; | ||
| string task_queue_name = 3; | ||
| int32 task_queue_type = 4; |
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.
We should be able to use
TaskQueueType from temporal/api/enums/v1/task_queue.proto and TaskType from temporal/server/api/enums/v1/task.proto.
| dynamicConfigClient.OverrideValue("matching.rps", 20) | ||
| dynamicConfigClient.OverrideValue("history.rps", 30) | ||
| dynamicConfigClient.OverrideValue("limit.maxIDLength", 255) | ||
| dynamicConfigClient.OverrideValue("history.enableTransitionHistory", true) |
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.
Test more complex configs here. Anything that has slices or maps or is a more complex struct. There are a few of those in dynamicconfig/constants.go.
| { | ||
| Name: "config", | ||
| Usage: "Command to get dynamic config values", | ||
| Aliases: []string{"cfg"}, |
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.
| Aliases: []string{"cfg"}, |
| return []*cli.Command{ | ||
| { | ||
| Name: "get", | ||
| Aliases: []string{"g"}, |
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.
| Aliases: []string{"g"}, |
| Flags: []cli.Flag{ | ||
| &cli.StringSliceFlag{ | ||
| Name: FlagKey, | ||
| Usage: "Enter keys", |
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.
| Usage: "Enter keys", | |
| Usage: "Dynamic config keys to get", |
|
|
||
| keys := c.StringSlice(FlagKey) | ||
| if len(keys) == 0 { | ||
| return fmt.Errorf("flag %q is required", FlagKey) |
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.
| return fmt.Errorf("flag %q is required", FlagKey) | |
| return fmt.Errorf("at least one %q flag is required", FlagKey) |
What changed?
Added an admin command that dump all frontend dynamic config values,
Why?
Operators currently cannot easily inspect frontend runtime dynamic configuration, dumping values aids troubleshooting, incident response, and verification after config changes.
How did you test it?
Closes #421