I don't know the history either, I quickly scanned the KIP-55 threads and couldn't see it being discussed.
Anyway, your suggestion sounds good to me, are you planning to do that as part of KIP-196 or should I create a new JIRA ? On Wed, Sep 27, 2017 at 3:20 AM, Ewen Cheslack-Postava <e...@confluent.io> wrote: > It's a fair point that it would undo that sanitization. It's possible that > for compatibility reasons, doing so would require a bit more work and care > (e.g. supporting both sanitized and unsanitized for awhile so users have a > chance to migrate). But I guess my point is that I view the location where > sanitization is happening as more of a bug, and I actually haven't followed > the whole history of that discussion so I'm not entirely sure whether it > was intentional or just sort of happened along with the sanitization > required for generating ZK paths. > > Anyway, I think we shouldn't choose to cripple all metrics just because the > metrics involving those user principals weren't handled ideally. If you're > open to this, we could always make the change to the code affected by this > KIP, leave the user principal metrics sanitized as they are today, and then > follow up with another KIP to get all the metrics unsanitized when they hit > the metrics reporter, but for Jmx sanitized as they are today when > characters are encountered that Jmx cannot support. > > -Ewen > > On Tue, Sep 26, 2017 at 2:24 AM, Mickael Maison <mickael.mai...@gmail.com> > wrote: > >> Hi Ewen, >> >> By consistency, I meant having all fields sanitized the same way we >> were previously doing for user principal. >> >> But re-reading your previous email, I'm guessing you meant to also >> remove the current user principal sanitization from the metrics (only >> use that internally for ZK) and have all metrics use the actual >> values. If so, yes that's an option. >> >> On Mon, Sep 25, 2017 at 5:58 PM, Ewen Cheslack-Postava >> <e...@confluent.io> wrote: >> > On Mon, Sep 25, 2017 at 3:35 AM, Mickael Maison < >> mickael.mai...@gmail.com> >> > wrote: >> > >> >> 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). >> >> >> > >> > How is just passing the actual values to the reporters any less >> consistent? >> > The "sanitization" process that was in place was really only for internal >> > purposes, right? i.e. so that we'd have a path for ZK that ZK could >> handle? >> > >> > I think the real question is why JmxReporter is being special-cased? >> > >> > >> >> >> >> Moving the sanitization into JMXReporter is a publicly visible change >> >> as it would affect the metrics we pass to other reporters. >> >> >> > >> > How would this be any more publicly visible than the change already being >> > made? In fact, since we haven't really specified what reporters should >> > accept, if anything the change to the sanitized strings is more of a >> > publicly visible change (you need to understand exactly what >> transformation >> > is being applied) than the change I am suggesting (which could be >> > considered a bug fix that now just fixes support for certain client IDs >> and >> > only affects JMX metric names because of JMX limitations). >> > >> > -Ewen >> > >> > >> >> >> >> >> >> >> >> >> >> 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 >> >> >> > > >> >> >> > >> >> >> >> >> >>
