dragosvictor commented on PR #22467: URL: https://github.com/apache/pulsar/pull/22467#issuecomment-2071044457
> Changes look good. > > My only comment is that we should be using qualified names in the attributes: > > > pulsar.namespace Includes the namespace portion only my-namespace > > pulsar.topic Includes the local topic name my-topic > > eg: `pulsar.namespace` -> `my-tenant/my-namespace` `pulsar.topic` -> `my-tenant/my-namespace/my-topic` > > This is to keep consistency with many other places in APIs and CLI tools. Also, consistent with OTel metrics attributes in the client SDK. Note that such a transformation would lead the namespace to occasionally include the "cluster" portion in case of old topic names: `pulsar.namespace="my-property/use/my-ns"` The topic name, as is currently filled in by the [client](https://github.com/apache/pulsar/blob/c72c135541e14043370836421cfef372b1d0a0ea/pulsar-client/src/main/java/org/apache/pulsar/client/impl/metrics/MetricsUtil.java#L47) also includes the persistence part: `pulsar.topic="persistent://my-property/use/my-ns/testAllCompactedOut-07b9ad7f-89cb-4800-88e8-cb3417cf0406"`. If we can confirm that this is the intent here, I can go ahead and proceed with this proposal. Alternatively, we can augment the existing implementation with one more attribute, say `pulsar.topic.complete.name`, even tough it leads to repetition. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
