-
Notifications
You must be signed in to change notification settings - Fork 5.5k
admin: add json serialized proto for /clusters #3478
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
Changes from 6 commits
ec9e498
e7b9bd8
94ac601
b0c76b2
cf51d8b
cf1ac78
4a72fd5
0a035f3
63a67f6
a5ce4cc
7e1133e
b60e1b1
e57410f
0989b2e
8a5e317
35717cc
d9a259d
f32ad14
a82839a
94a23cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,77 @@ | ||||
| syntax = "proto3"; | ||||
|
|
||||
| package envoy.admin.v2alpha; | ||||
|
|
||||
| import "envoy/api/v2/core/base.proto"; | ||||
|
|
||||
| // [#protodoc-title: Clusters] | ||||
|
|
||||
| // Admin endpoint uses this wrapper for `/clusters` to display cluster status information. | ||||
| // See :ref:`/clusters <operations_admin_interface_clusters>` for more information. | ||||
| message Clusters { | ||||
| // Mapping from cluster name to each cluster's status. | ||||
| map<string, ClusterStatus> cluster_statuses = 1; | ||||
| } | ||||
|
|
||||
| // Details an individual cluster's current status. | ||||
| message ClusterStatus { | ||||
| // General outlier statistics if installed for this cluster. | ||||
| OutlierInfo outlier_info = 1; | ||||
|
|
||||
| // Mapping from priority to circuit breaker settings for that priority. | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style nit: I'd prefer a blank line between fields for readability. |
||||
| map<string, CircuitSettings> circuit_settings = 2; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want envoy/api/envoy/api/v2/core/base.proto Line 114 in 49b122d
|
||||
|
|
||||
| // Denotes whether this cluster was added via API or configured statically. | ||||
| bool added_via_api = 3; | ||||
|
|
||||
| // Mapping from host address to the host's current status. | ||||
| map<string, HostStatus> host_statuses = 4; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to use https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/core/address.proto#L93 here? I.e. have some structure since this is proto, rather than encoding ports etc. in strings.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This brings up a broader question. I think I have made uncharacteristic (in the scope of Envoy) use of maps. Both messages and enums (as mentioned in your other comment below) cannot be used as key types. Should we try to avoid maps here, generally (even when the key types allow), or just remove them in these two incompatible places?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it probably makes sense for the |
||||
| } | ||||
|
|
||||
| // Cluster outlier detection statistics. -1 for any of these statistics denotes that there was not | ||||
| // enough data to compute it. | ||||
| message OutlierInfo { | ||||
| // The average success rate of the hosts in the Detector for the last aggregation interval. | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. References back to docs on this? |
||||
| double success_rate_average = 1; | ||||
|
|
||||
| // The success rate threshold used in the last interval. The threshold is used to eject hosts | ||||
| // based on their success rate. | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these percentages/ratios? If so, consider expressing with https://github.com/envoyproxy/envoy/blob/master/api/envoy/type/percent.proto. |
||||
| double success_rate_ejection_threshold = 2; | ||||
| } | ||||
|
|
||||
| // Circuit settings for a particular priority setting. | ||||
| message CircuitSettings { | ||||
| // Total TCP connection limit. | ||||
| int64 max_connections = 1; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Envoy, we prefer unsigned types for things that are actually unsigned. |
||||
|
|
||||
| // Pending request limit. A request is pending if it is waiting to be attached to a connection | ||||
| // pool connection. | ||||
| int64 max_pending_requests = 2; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't these
/config_dump, so can we elide here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to be consistent with the info provided in the text printout, but I'll remove as long as there are no back-compat objections. |
||||
|
|
||||
| // Active request limit. A request is active if it is bound to a connection pool connection and | ||||
| // awaiting a response. | ||||
| int64 max_requests = 3; | ||||
|
|
||||
| // Per-request retry limit. | ||||
| int64 max_retries = 4; | ||||
| } | ||||
|
|
||||
| message HostStatus { | ||||
| // Mapping from the name of the statistic to the current value. | ||||
| map<string, int64> stats = 1; | ||||
|
|
||||
| // A string representation of the host's current health status. | ||||
| string health_flags = 2; | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer proto structure rather than opaque strings here. |
||||
|
|
||||
| // Configured load balancing weight for this host. | ||||
| int64 weight = 3; | ||||
|
|
||||
| // Configured locality for the host. | ||||
| envoy.api.v2.core.Locality locality = 4; | ||||
|
|
||||
| // Denotes whether this is configured as a canary host or not. | ||||
| bool canary = 5; | ||||
|
|
||||
| // Request success rate for this host over the last calculated interval. | ||||
| double success_rate = 6; | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,3 +6,4 @@ Admin | |
| :maxdepth: 2 | ||
|
|
||
| ../admin/v2alpha/config_dump.proto | ||
| ../admin/v2alpha/clusters.proto | ||
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.
Can you reorder the protos so that they are in topological sort order? This make the documentation clearer and is a convention we have adopted.
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 to be clear, you're suggesting that if one message depends on another, the dependent message should go below the dependency? Is this a new convention? I was following the example of the bootstrap proto and others, which seem to do the opposite.
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.
Actually, I think I'm wrong here, the other opposite convention seems to mostly hold.