Can the Discussion Thread link be filled out ? Cheers
On Wed, Sep 27, 2017 at 2:03 AM, Mickael Maison <mickael.mai...@gmail.com> wrote: > 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 > >> >> >> > > > >> >> >> > > >> >> >> > >> >> > >> >
