Kafka's clientID to default to appID

[artursouza]

Today, Kafka's pubsub and binding default clientID to sarama - which is useless. Since clientID is used by Kafka to apply quotas and set in tracing, defaulting to Dapr's appID make more sense.

https://docs.dapr.io/reference/components-reference/supported-pubsub/setup-apache-kafka/#spec-metadata-fields

Related to: https://github.com/dapr/dapr/issues/5690

[VerstraeteBert]

@artursouza would it make sense to default it to {namespace}? This translates to $"{namespace}.{appID}". Is appID enforced to be unique over namespaces? If not, {namespace} seems to be the most precise, human-readable identifier. Aside, namespace is quite an unlucky name for the template string, as it's more than that, too late to change something as trivial as this?

[yaron2]

appID is not enforced to be unique across namespaces. however, when running in self-hosted mode (outside of K8s), Dapr by default does not have a namespace configured.

[VerstraeteBert]

Thanks for the explanation. Makes sense to me to have {appId} as the clientId in self-hosted mode and {namespace} in k8s 👍 . I'll get on it during the weekend.

[yaron2]

Thanks for the explanation. Makes sense to me to have {appId} as the clientId in self-hosted mode and {namespace} in k8s 👍 . I'll get on it during the weekend.

@artursouza does this seem reasonable to you? looks fine to me.

[artursouza]

LGTM

[artursouza]

@artursouza would it make sense to default it to {namespace}? This translates to $"{namespace}.{appID}". Is appID enforced to be unique over namespaces? If not, {namespace} seems to be the most precise, human-readable identifier. Aside, namespace is quite an unlucky name for the template string, as it's more than that, too late to change something as trivial as this?

I am not a fan of {namespace} implementation either. I think we should have a deprecation notice to that and change the implementation to be just the actual namespace - although it is not portable when running outside K8s.

[artursouza]

After this change is in. I propose we simply make a breaking change where {namespace} means the actual namespace only and existing users should use {appId} concatenated going forward to keep existing behavior.

[berndverst]

App ID and namespace aren't available in contrib right now.

Can this be added to the context object we use everywhere?

Or alternatively runtime can inject this as specially prefixed metadata (properties) keys which then can be access in every component!

Not sure how I feel about a component making an extra API call to retrieve the app ID and namespace - I'm personally against this because it also introduces new failure scenarios.

[artursouza]

Some of the discussion here is about https://github.com/dapr/dapr/issues/5690 - for contrib, as long as more context is passed to components (like consumerID), then each component can use those for "better" default value in some properties.

[berndverst]

Yes to set the better defaults in the component the app ID and namespace need to be accessible. That can only be done by injecting them into the metadata properties map within runtime. Each component's Init message has access to these.

[VerstraeteBert]

@berndverst @artursouza thanks for the discussion and pointers. Will delve deeper into the code soon.

[artursouza]

I will look into this to validate the latest state of Kafka.

[cicoyle]

Validated, but waiting on docs PR ^ to be merged before closing this issue

[cicoyle]

/assign