Conversation
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
The overall design looks good, but I think the doc needs work.
My main concern is that we need a much better description of the overall design in a language-agnostic way, rather than just saying that we have a different design for each language. It's true that each language has its own APIs for how per-channel options are set, but the overall goal here is still to have a way to pass a set of options to be used in child channels. The semantics for that should be the same in all languages.
Please let me know if you have any questions. Thanks!
A110-child-channel-plugins.md
Outdated
| * StatsPlugins & Tracing: Users need to configure metric sinks (as described in gRFC [A66](https://github.com/grpc/proposal/blob/master/A66-otel-stats.md) and [A72](https://github.com/grpc/proposal/blob/master/A72-open-telemetry-tracing.md)) so that telemetry from internal channels is correctly tagged and exported. | ||
| * Interceptors: Users may need to apply specific interceptors (e.g., for authentication, logging, or tracing) to internal traffic. | ||
|
|
||
| These configurations cannot be set globally because different parts of an application may require different configurations, such as different metric backends or security credentials. |
There was a problem hiding this comment.
Or we just mention the reason we have this problem: libraries. Libraries want to configure their channels, but can't configure globally.
There was a problem hiding this comment.
This is there in rationale.
A110-child-channel-plugins.md
Outdated
| 2. It applies `O_child` to the configuration of `C`. | ||
|
|
||
| ### Precedence and Merging | ||
| The Child Channel `C` typically requires some internal configuration `O_internal` (e.g., specific target URIs, bootstrap credentials, or internal User-Agent strings). |
There was a problem hiding this comment.
There's a lot of internal configuration. In the Java design, some would be able to be changed by O_child, but other parts not. I really don't know what this specifically is trying to call out. This set of examples here doesn't help me much, as Java wouldn't let you change the target URI or credentials, and we have no case where we're setting an internal User-Agent string.
| // TODO(AgraVator): add example for configuring child channel observability | ||
| ``` | ||
|
|
||
| ## Rationale |
There was a problem hiding this comment.
We need to say somewhere that we are not trying to provide any identity information about which channel is being created. The configuration provided by this gRFC is required to be uniform across child channels of a particular channel.
There was a problem hiding this comment.
Are we sure this should be a non-goal? E.g. we might have xds client connections needing a different set of options from RLS connections/etc. It seems pretty plausible given that one team will own the control plane, but a different team would own the ext_proc services, so different credentials might be needed or different telemetry configuration may be desired.
This doesn't have to be hard; it could be a simple map from name->config.
There was a problem hiding this comment.
This is not for credentials. Both of those channels would send telemetry to the same destination (configured here), so they'd use the same credentials. We would solve multi-ownership problems by allowing applying telemetry multiple times, a different time for each consumer/configuration.
I don't disagree that someone may want to identify the purpose of the child channel, but that then means we need some way to identify the purpose of the child channel. That's a bigger lift and opens up lots of complexities on how you will categorize the child channels' purposes. In our TL discussion about it, nobody could come up with a strong need for the info, we so left it for future work if/when it is needed.
A110-child-channel-plugins.md
Outdated
| 1. It retrieves `O_child` from `P`. | ||
| 2. It applies `O_child` to the configuration of `C`. | ||
|
|
||
| ### Precedence and Merging |
There was a problem hiding this comment.
We should say somewhere that the child channel options override global channel options. Although I hope in practice that doesn't really matter much.
A110-child-channel-plugins.md
Outdated
| * Interceptors: Users may need to apply specific interceptors (e.g., for authentication, logging, or tracing) to internal traffic. | ||
| * Interceptors: Users may need to apply specific interceptors (e.g., for logging, or tracing) to internal traffic. | ||
|
|
||
| These configurations cannot be set globally because different parts of an application may require different configurations, such as different metric backends or security credentials. |
There was a problem hiding this comment.
"security credentials" is still here, even though we're saying not to use this for authentication any more.
| // TODO(AgraVator): add example for configuring child channel observability | ||
| ``` | ||
|
|
||
| ## Rationale |
There was a problem hiding this comment.
This is not for credentials. Both of those channels would send telemetry to the same destination (configured here), so they'd use the same credentials. We would solve multi-ownership problems by allowing applying telemetry multiple times, a different time for each consumer/configuration.
I don't disagree that someone may want to identify the purpose of the child channel, but that then means we need some way to identify the purpose of the child channel. That's a bigger lift and opens up lots of complexities on how you will categorize the child channels' purposes. In our TL discussion about it, nobody could come up with a strong need for the info, we so left it for future work if/when it is needed.
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This is looking much better!
Please let me know if you have any questions. Thanks!
7 participants
No description provided.