Re: [DISCUSS] KIP-190: Handle client-ids consistently between clients and brokers

[Ted Yu <yu...@gmail.com>]

Can the Discussion Thread link be filled out ?

Cheers

On Wed, Sep 27, 2017 at 2:03 AM, Mickael Maison <mi...@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
> <ew...@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.maison@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
> >> <ew...@confluent.io> wrote:
> >> > On Mon, Sep 25, 2017 at 3:35 AM, Mickael Maison <
> >> mickael.maison@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
> >> >> <ew...@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.bentley@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 <is...@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.maison@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
> >> >> >> > >
> >> >> >> >
> >> >> >>
> >> >>
> >>
>