Hi Ewen, Thanks for the feedback!
If that's the case, would it maybe be possible to compatibly change the > default to use task IDs in the client ID, but only if we don't see an > existing override from the worker config? This would only change the > behavior when someone is using the default, but since the default would > just use what is effectively a random ID that is useless for monitoring > metrics, presumably this wouldn't affect any existing users. I think that > would avoid having to introduce the config, give better out of the box > behavior, and still be a safe, compatible change to make. Yes, it would be possible to simply change the default. There were two reasons I went with a config option in the proposal. I was primarily trying to avoid giving users a surprise by altering existing behavior. But, as you said, people are unlikely be relying on the current random default for monitoring so this might be fine. Another minor advantage of my proposal is it keeps open the option of setting arbitrary custom client IDs. If we only append the task ID to the default client ID then setting the "producer.clientid" or "consumer.clientid" will always result in a name conflict and will almost always need to be avoided. If a change in the default behavior is acceptable, and we're OK with advising people to avoid setting "producer.clientid" or "consumer.clientid" in future, then I'm happy to go with your suggested approach of changing the default. ... or change the type to be an > enum and make it something like task.client.ids=[default|task] and leave it > open for extension in the future if needed). If we were going with new config, I like the enum option. > *"Allow overriding client.id <http://client.id/> on a per-connector > basis"* > > > > This is a much more complex change, and would require individual > > connectors to be updated to support the change. In contrast, the proposed > > approach would immediately allow detailed consumer/producer monitoring > for > > all existing connectors. > > > I don't think this is quite accurate. I think the reason to reject is that > for your particular requirement for metrics, it simply doesn't give enough > granularity (there's only one value per entire connector), but it doesn't > ... > config value (as we now do with managed secrets). For example, it could > support something like client.id=connector-${taskId} and the task ID would > be substituted automatically into the string. > Yes, I think I was misinterpreting "on a per-connector basis" to mean giving each connector a way to override client properties on a per-task basis: a simple per-connector override clearly wouldn't work. I was imagining adding a way to intercept and modify the client properties before they're used by the Worker to construct the client. You're right that this wouldn't necessarily need to live in the connector. Config interpolation is an option, as you mentioned. Either way, this is certainly a more complex change. Paul
