Hi Ewen, I understand your point of view and ideally we'd have one convention for handling all user provided strings. This KIP reused the sanitization mechanism we had in place for user principals.
I think both ways have pros and cons but what I like about early sanitization (as is currently) is that it's consistent. While monitoring tools have to know about the sanitization, all metrics are sanitized the same way before being passed to reporters and that includes all "fields" in the metric name (client-id, user). Moving the sanitization into JMXReporter is a publicly visible change as it would affect the metrics we pass to other reporters. On Fri, Sep 22, 2017 at 8:53 PM, Ewen Cheslack-Postava <e...@confluent.io> wrote: > Hi all, > > In working on the patch for KIP-196: Add metrics to Kafka Connect > framework, we realized that we have worker and connector/task IDs that are > to be included in metrics and those don't currently have constraints on > naming. I'd prefer to avoid adding naming restrictions or mangling names > unnecessarily, and for users that define a custom metrics reporter the name > mangling may be unexpected since their metrics system may not have the same > limitations as JMX. > > The text of the KIP is pretty JMX specific and doesn't really define where > this mangling happens. Currently, it is being applied essentially as early > as possible. I would like to propose moving the name mangling into the > JmxReporter itself so the only impact is on JMX metrics, but other metrics > reporters would see the original. In other words, leave system-specific > name mangling up to that system. > > In the JmxReporter, the mangling could remain the same (though I think the > mangling for principals is an internal implementation detail, whereas this > name mangling is user-visible). The quota metrics on the broker would now > be inconsistent with the others, but I think trying to be less JMX-specific > given that we support pluggable reporters is the right direction to go. > > I think in practice this has no publicly visible impact and definitely no > compatibility concerns, it just moves where we're doing the JMX name > mangling. However, since discussion about metric naming/character > substitutions had happened here recently I wanted to raise it here and make > sure there would be agreement on this direction. > > (Long term I'd also like to get the required instantiation of JmxReporter > removed as well, but that requires its own KIP.) > > Thanks, > Ewen > > On Thu, Sep 14, 2017 at 2:09 AM, Tom Bentley <t.j.bent...@gmail.com> wrote: > >> Hi Mickael, >> >> I was just wondering why the restriction was imposed for Java clients the >> first place, do you know? >> >> Cheers, >> >> Tom >> >> On 14 September 2017 at 09:16, Ismael Juma <ism...@juma.me.uk> wrote: >> >> > Thanks for the KIP Mickael. I suggest starting a vote. >> > >> > Ismael >> > >> > On Mon, Aug 21, 2017 at 2:51 PM, Mickael Maison < >> mickael.mai...@gmail.com> >> > wrote: >> > >> > > Hi all, >> > > >> > > I have created a KIP to cleanup the way client-ids are handled by >> > > brokers and clients. >> > > >> > > Currently the Java clients have some restrictions on the client-ids >> > > that are not enforced by the brokers. Using 3rd party clients, >> > > client-ids containing any characters can be used causing some strange >> > > behaviours in the way brokers handle metrics and quotas. >> > > >> > > Feedback is appreciated. >> > > >> > > Thanks >> > > >> > >>
