-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add command to dump frontend dynamic config values #8214
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
Add command to dump frontend dynamic config values #8214
Conversation
|
This isn't dumping all dynamic config values, it's only dumping the frontend config, and only for one particular (nonexistent) namespace. To do this properly you'd need to add support in the dynamicconfig.Client interface, but I'm not sure we want to do that. We could support getting the value of a single key by name without changing the interface. What are you actually trying to use this for? |
|
@dnr, @vaibhavyadav-dev joined the community slack and volunteered to contribute. I gave him this issue since I knew there's been a longstanding ask to add this API. 100% agree with the feedback so far. I can guide from here. |
bergundy
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.
You are going to want to also add a ListKeys() method to the dynamic config client.
@dnr do you think this is a reasonable request?
|
|
||
| rpc ForceUnloadTaskQueuePartition (ForceUnloadTaskQueuePartitionRequest) returns (ForceUnloadTaskQueuePartitionResponse) {} | ||
|
|
||
| rpc GetConfigurations(GetConfigurationsRequest) returns (GetConfigurationsResponse) {} |
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.
| rpc GetConfigurations(GetConfigurationsRequest) returns (GetConfigurationsResponse) {} | |
| rpc GetConfiguration(GetConfigurationRequest) returns (GetConfigurationResponse) {} |
| message GetConfigurationsRequest {} | ||
| message GetConfigurationsResponse{ |
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.
| message GetConfigurationsRequest {} | |
| message GetConfigurationsResponse{ | |
| message GetConfigurationRequest {} | |
| message GetConfigurationResponse { |
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 make all this stuff have the string "DynamicConfig" in it. There are two types of configuration and we should be explicit to avoid confusion. (I would even prefer GetDynamicConfig to GetDynamicConfiguration since that's how it's referred to all over the code.)
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.
okay
| string cluster_name = 1; | ||
| map<string, google.protobuf.Value> config_values = 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.
| string cluster_name = 1; | |
| map<string, google.protobuf.Value> config_values = 2; | |
| repeated HostConfig host_config = 1; |
And add a HostConfig message that will embed structured config via a struct:
message HostConfig {
// The host that this configuration was loaded from.
string hostname = 1;
// The raw dynamic config loaded with all constraints.
google.protobuf.Struct raw_dynamic_config = 2;
}
I would rather not. I don't think we should add methods there without a very good reason, and I don't feel like this is it. I think we should support a single key query to start and then see if it satisfies people's use case. (Tbh I'm skeptical this is even useful at all but I guess it doesn't hurt.) The problem is that it'll break the build for anyone with a custom Client until they add support. Also just ListKeys plus calling GetValue on each key is racy (a key may be added or removed between the List and the Get). So you want to get a snapshot of the whole state, but that may be expensive. If we really do need to do that, we should add it as optional (i.e. interface-assert it when needed) to not interfere with existing Clients. |
Curious if there are known client implementations that we would be concerned with here or if this is mostly speculation. |
|
Lets make You will need to copy the definition of message ConstrainedValue {
Constraints constraints = 1;
google.protobuf.Struct value = 2;
}
message HostConfig {
// The host that this configuration was loaded from.
string hostname = 1;
// The raw dynamic config loaded with all constraints.
map<string, ConstrainedValue> dynamic_config = 2;
} |
|
Sorry if I mislead you, each config name may have multiple constrained values, this would be a better representation: message ConstrainedValue {
Constraints constraints = 1;
google.protobuf.Struct value = 2;
}
message ConstrainedValues {
repeated ConstrainedValue items = 1;
}
message HostConfig {
// The host that this configuration was loaded from.
string hostname = 1;
// The raw dynamic config loaded with all constraints.
map<string, ConstrainedValues> dynamic_config = 2;
} |
e90aa6c to
adbce8b
Compare
| message Constraints { | ||
| string namespace = 1; | ||
| string namespace_id = 2; | ||
| string task_queue_name = 3; | ||
| int32 task_queue_type = 4; | ||
| int32 shard_id = 5; | ||
| int32 task_type = 6; | ||
| string destination = 7; | ||
| } | ||
|
|
||
| message ConstrainedValue { | ||
| Constraints constraints = 1; | ||
| google.protobuf.Struct value = 2; | ||
| } | ||
|
|
||
| message ConstrainedValues { | ||
| repeated ConstrainedValue items = 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.
You can put this in proto/internal/temporal/server/api/dynamicconfig/v1/message.proto.
|
|
||
| rpc ForceUnloadTaskQueuePartition (ForceUnloadTaskQueuePartitionRequest) returns (ForceUnloadTaskQueuePartitionResponse) {} | ||
|
|
||
| rpc GetDynamicConfigurations(GetDynamicConfigurationsRequest) returns (GetDynamicConfigurationsResponse) {} |
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.
| rpc GetDynamicConfigurations(GetDynamicConfigurationsRequest) returns (GetDynamicConfigurationsResponse) {} | |
| rpc GetHostConfiguration(GetHostConfigurationRequest) returns (GetHostConfigurationResponse) {} |
service/frontend/admin_handler.go
Outdated
| pageSize := 100 | ||
| var nextPageToken []byte | ||
|
|
||
| namespaces := make([]string, 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.
You don't need to get the list of namespaces to implement this feature.
service/frontend/admin_handler.go
Outdated
| constrainedValues := adh.getConstrainedValueForKey(key, namespaces) | ||
| if len(constrainedValues) > 0 { | ||
| dynamicConfig[key] = &adminservice.ConstrainedValues{ | ||
| Items: constrainedValues, | ||
| } | ||
| } |
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 would just be:
dcClient.GetValue(key)
Just make sure you have access to a dynamicconfig.Client instance in this handler.
36962cc to
2cd597c
Compare
|
Closing this PR; opening a fresh one with a cleaner commit history. |
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