You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Dániel Urbán <ur...@gmail.com> on 2023/03/30 16:24:07 UTC

[DISCUSS] KIP-916: MM2 distributed mode flow log context

Hello everyone,

I would like to kick off a discussion about KIP-916:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-916%3A+MM2+distributed+mode+flow+log+context

The KIP aims to enhance the diagnostic information for MM2 distributed
mode. MM2 relies on multiple Connect worker instances nested in a single
process. In Connect, Connector names are guaranteed to be unique in a
single process, but in MM2, this is not true. Because of this, the
diagnostics provided by Connect (client.ids, log context) do not ensure
that logs are distinguishable for different flows (Connect workers) inside
an MM2 process.

Thanks for all you input in advance,
Daniel

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Greg Harris <gr...@aiven.io.INVALID>.
Hey Daniel,

Thanks for the KIP! The current logging behavior seems like a
usability nightmare. I've only debugged single-worker JVM deployments,
and I see now that I took the uniqueness of a connector name for
granted.

I had a few questions about the change:

1. The MDC context change and the thread name change both seem to
solve the problem, and I assume that the majority of people would find
the thread name sufficient and leave things at their default. What is
the advantage of implementing both vs picking one?

2. I see that there is already a `workerId` that is passed around for
emitting to the status topic and to namespace the metrics. In MM2
dedicated mode, this contains the source and target. Do you think it
could be used to disambiguate the log context for
WorkerConnector/WorkerSinkTask as well?

3. Is the 'flow' (source->target) always more relevant than the
'source' and 'target' separately? Does it ever make sense to aggregate
logs for one source and multiple targets, or vice versa, and should we
expose these fields separately?

Overall, I think this feels like a shared-JVM problem that just
happens to appear in MM2 dedicated mode. I am interested to see if
there is a solution to this problem that also makes debugging easier
for other shared-JVM use-cases. We've also solved it in a number of
different ways and I wonder if there's a holistic way to namespace the
logging, metrics, and threads for workers in a shared JVM.

Thanks!
Greg

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
If there are no further comments, I will kick off a vote soon for the KIP.

Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún. 12., H,
11:27):

> Updated the KIP with a few example log lines before/after the change.
> Daniel
>
> Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún. 7.,
> Sze, 13:59):
>
>> Hi Chris,
>>
>> Thank you for your comments! I updated the KIP. I still need to add the
>> example before/after log lines, will do that soon, but I addressed all the
>> other points.
>> 1. Added more details on thread renaming under Public Interfaces, removed
>> pseudo code.
>> 2. Removed the stale header - originally, client.id related changes were
>> part of the KIP, and I failed removing all leftovers of that version.
>> 3. Threads listed under Public Interfaces with current/proposed names.
>> 4. Added a comment in the connect-log4j.properties, similar to the one
>> introduced in KIP-449. We don't have a dedicated MM2 log4j config, not sure
>> if we should introduce it here.
>> 5. Clarified testing section - I think thread names should not be tested
>> (they never were), but testing will focus on the new MDC context value.
>>
>> Thanks,
>> Daniel
>>
>> Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún.
>> 5., H, 16:46):
>>
>>> Hi Daniel,
>>>
>>> Thanks for the updates! A few more thoughts:
>>>
>>> 1. The "Proposed Changes" section seems a bit like a pseudocode
>>> implementation of the KIP. We don't really need this level of detail;
>>> most
>>> of the time, we're just looking for implementation details that don't
>>> directly affect the user-facing changes proposed in the "Public
>>> Interfaces"
>>> section but are worth mentioning because they (1) demonstrate how the
>>> user-facing changes are made possible, (2) indirectly affect user-facing
>>> behavior, or (3) go into more detail (like providing examples) about the
>>> user-facing behavior. For this KIP, I think examples of user-facing
>>> behavior (like before/after of thread names and log messages) and
>>> possibly
>>> an official description of the scope of the changes (which threads are
>>> going to be renamed and/or include the new MDC key, and which aren't?)
>>> are
>>> all that we'd really need in this section; everything else is fairly
>>> clear
>>> IMO. FWIW, the reason we want to discourage going into too much detail
>>> with
>>> KIPs is that it can quickly devolve into code review over mailing list,
>>> which can hold KIPs up for longer than necessary when the core design
>>> changes they contain are already basically accepted by everyone.
>>>
>>> 2. The "MM2 distributed mode client.id and log change" header seems
>>> like it
>>> may no longer be accurate; the contents do not mention any changes to
>>> client IDs. I might be missing something though; please correct me if I
>>> am.
>>>
>>> 3. Can you provide some before/after examples of what thread names and
>>> log
>>> messages will look like? I'm wondering about the thread that the
>>> distributed herder runs on, threads for connectors and tasks, and threads
>>> for polling internal topics (which we currently manage with the
>>> KafkaBasedLog class). It's fine if some of these are unchanged, I just
>>> want
>>> to better understand the scope of the proposed changes and get an idea of
>>> how they may appear to users.
>>>
>>> 4. There's no mention of changes to the default Log4j config files that
>>> we
>>> ship. Is this intentional? I feel like we need some way for users to
>>> easily
>>> discover this feature; if we're not going to touch our default Log4j
>>> config
>>> files, is there another way that we can expect users to find out about
>>> the
>>> new MDC key?
>>>
>>> 5. RE the "Test Plan" section: can you provide a little more detail of
>>> which cases we'll be covering with the proposed unit tests? Will we be
>>> verifying that the MDC context is set in various places? If so, which
>>> places? And the same with thread names? (There doesn't have to be a ton
>>> of
>>> detail, but a little more than "unit tests" would be nice 😄)
>>>
>>> Cheers,
>>>
>>> Chris
>>>
>>> On Mon, Jun 5, 2023 at 9:45 AM Dániel Urbán <ur...@gmail.com>
>>> wrote:
>>>
>>> > I updated the KIP accordingly. Tried to come up with more generic
>>> terms in
>>> > the Connect code instead of referring to "flow" anywhere.
>>> > Daniel
>>> >
>>> > Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún.
>>> 5., H,
>>> > 14:49):
>>> >
>>> > > Hi Chris,
>>> > >
>>> > > Thank you for your comments!
>>> > >
>>> > > I agree that the toString based logging is not ideal, and I believe
>>> all
>>> > > occurrences are within a proper logging context, so they can be
>>> ignored.
>>> > > If thread names can be changed unconditionally, I agree, using a new
>>> MDC
>>> > > key is the ideal solution.
>>> > >
>>> > > Will update the KIP accordingly.
>>> > >
>>> > > Thanks,
>>> > > Daniel
>>> > >
>>> > > Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023.
>>> jún.
>>> > 2.,
>>> > > P, 22:23):
>>> > >
>>> > >> Hi Daniel,
>>> > >>
>>> > >> Are there any cases of Object::toString being used by internal
>>> classes
>>> > for
>>> > >> logging context that can't be covered by MDC contexts? For example,
>>> > >> anything logged by WorkerSinkTask (or any WorkerTask subclass)
>>> already
>>> > has
>>> > >> the MDC set for the task [1]. Since the Object::toString-prefixed
>>> style
>>> > of
>>> > >> logging is a bit obsolete after the introduction of MDC contexts
>>> it'd
>>> > feel
>>> > >> a little strange to continue trying to accommodate it, especially
>>> if the
>>> > >> changes from this KIP are going to be opt-in regardless.
>>> > >>
>>> > >> As far as thread names go: unlike log statements, I don't believe
>>> > changing
>>> > >> them requires a KIP, and would be fine with merging a PR for that
>>> > without
>>> > >> worrying about compatibility.
>>> > >>
>>> > >> With that in mind, I'm still wondering if we can add a separate MDC
>>> key
>>> > >> for
>>> > >> MM2 replication flows (perhaps "mirror.maker.flow") and
>>> unconditionally
>>> > >> add
>>> > >> that to the MDC contexts for every thread that gets spun up by each
>>> > >> DistributedHerder instance that MM2 creates. This would be different
>>> > from
>>> > >> the draft PR and KIP, which involve altering the content of the
>>> existing
>>> > >> "connector.context" MDC key. Users would opt in to the change by
>>> > altering
>>> > >> their Log4j config instead of altering their MM2 config, which
>>> matches
>>> > >> precedent set with the introduction of the "connector.context" key.
>>> > >>
>>> > >> Thoughts?
>>> > >>
>>> > >> [1] -
>>> > >>
>>> > >>
>>> >
>>> https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255
>>> > >>
>>> > >> Cheers,
>>> > >>
>>> > >> Chris
>>> > >>
>>> > >> On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <
>>> urb.daniel7@gmail.com>
>>> > >> wrote:
>>> > >>
>>> > >> > Hi Chris,
>>> > >> >
>>> > >> > Thank you for your comments!
>>> > >> > 1. This approach is OK for me. I thought that the "sample"
>>> configs in
>>> > >> the
>>> > >> > repo do not fall into the same category as the default of a new
>>> > config.
>>> > >> > Adding a commented line instead should be ok, and the future
>>> opt-out
>>> > >> change
>>> > >> > sounds good to me.
>>> > >> >
>>> > >> > 2. Besides the MDC, there are 2 other places where the flow
>>> context
>>> > >> > information could be added:
>>> > >> > - Some of the Connect internal classes use their .toString methods
>>> > when
>>> > >> > logging (e.g. WorkerSinkTask has a line like this: log.info("{}
>>> > >> Executing
>>> > >> > sink task", this);). These toString implementations contain the
>>> > >> connector
>>> > >> > name, and nothing else, so in MM2 dedicated mode, adding the flow
>>> > would
>>> > >> > make these lines unique.
>>> > >> > - Connect worker thread names. Currently, they contain the
>>> connector
>>> > >> > name/task ID, but to make them unique, the flow should be added
>>> in MM2
>>> > >> > distributed mode.
>>> > >> > In my draft PR, I changed both of these, but for the sake of
>>> backward
>>> > >> > compatibility, I made the new toString/thread name dependent on
>>> the
>>> > new,
>>> > >> > suggested configuration flag.
>>> > >> > If we go with a new MDC key, but we still want to change the
>>> toString
>>> > >> > methods and the thread names, we still might need an extra flag to
>>> > turn
>>> > >> on
>>> > >> > the new behavior.
>>> > >> > AFAIK the toString calls are all made inside a proper logging
>>> context,
>>> > >> so
>>> > >> > changing the toString methods don't add much value. I think that
>>> the
>>> > >> thread
>>> > >> > name changes are useful, giving more context for log lines written
>>> > >> outside
>>> > >> > of a log context.
>>> > >> >
>>> > >> > In short, I think that MDC + thread name changes are necessary to
>>> make
>>> > >> MM2
>>> > >> > dedicated logs useful for diagnostics. If we keep both, then maybe
>>> > >> having a
>>> > >> > single config to control both at the same time is better than
>>> having a
>>> > >> new
>>> > >> > MDC key (configured separately in the log pattern) and a config
>>> flag
>>> > >> (set
>>> > >> > separately in the properties file) for the thread name change.
>>> > >> >
>>> > >> > Thanks,
>>> > >> > Daniel
>>> > >> >
>>> > >>
>>> > >
>>> >
>>>
>>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
Updated the KIP with a few example log lines before/after the change.
Daniel

Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún. 7., Sze,
13:59):

> Hi Chris,
>
> Thank you for your comments! I updated the KIP. I still need to add the
> example before/after log lines, will do that soon, but I addressed all the
> other points.
> 1. Added more details on thread renaming under Public Interfaces, removed
> pseudo code.
> 2. Removed the stale header - originally, client.id related changes were
> part of the KIP, and I failed removing all leftovers of that version.
> 3. Threads listed under Public Interfaces with current/proposed names.
> 4. Added a comment in the connect-log4j.properties, similar to the one
> introduced in KIP-449. We don't have a dedicated MM2 log4j config, not sure
> if we should introduce it here.
> 5. Clarified testing section - I think thread names should not be tested
> (they never were), but testing will focus on the new MDC context value.
>
> Thanks,
> Daniel
>
> Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún. 5.,
> H, 16:46):
>
>> Hi Daniel,
>>
>> Thanks for the updates! A few more thoughts:
>>
>> 1. The "Proposed Changes" section seems a bit like a pseudocode
>> implementation of the KIP. We don't really need this level of detail; most
>> of the time, we're just looking for implementation details that don't
>> directly affect the user-facing changes proposed in the "Public
>> Interfaces"
>> section but are worth mentioning because they (1) demonstrate how the
>> user-facing changes are made possible, (2) indirectly affect user-facing
>> behavior, or (3) go into more detail (like providing examples) about the
>> user-facing behavior. For this KIP, I think examples of user-facing
>> behavior (like before/after of thread names and log messages) and possibly
>> an official description of the scope of the changes (which threads are
>> going to be renamed and/or include the new MDC key, and which aren't?) are
>> all that we'd really need in this section; everything else is fairly clear
>> IMO. FWIW, the reason we want to discourage going into too much detail
>> with
>> KIPs is that it can quickly devolve into code review over mailing list,
>> which can hold KIPs up for longer than necessary when the core design
>> changes they contain are already basically accepted by everyone.
>>
>> 2. The "MM2 distributed mode client.id and log change" header seems like
>> it
>> may no longer be accurate; the contents do not mention any changes to
>> client IDs. I might be missing something though; please correct me if I
>> am.
>>
>> 3. Can you provide some before/after examples of what thread names and log
>> messages will look like? I'm wondering about the thread that the
>> distributed herder runs on, threads for connectors and tasks, and threads
>> for polling internal topics (which we currently manage with the
>> KafkaBasedLog class). It's fine if some of these are unchanged, I just
>> want
>> to better understand the scope of the proposed changes and get an idea of
>> how they may appear to users.
>>
>> 4. There's no mention of changes to the default Log4j config files that we
>> ship. Is this intentional? I feel like we need some way for users to
>> easily
>> discover this feature; if we're not going to touch our default Log4j
>> config
>> files, is there another way that we can expect users to find out about the
>> new MDC key?
>>
>> 5. RE the "Test Plan" section: can you provide a little more detail of
>> which cases we'll be covering with the proposed unit tests? Will we be
>> verifying that the MDC context is set in various places? If so, which
>> places? And the same with thread names? (There doesn't have to be a ton of
>> detail, but a little more than "unit tests" would be nice 😄)
>>
>> Cheers,
>>
>> Chris
>>
>> On Mon, Jun 5, 2023 at 9:45 AM Dániel Urbán <ur...@gmail.com>
>> wrote:
>>
>> > I updated the KIP accordingly. Tried to come up with more generic terms
>> in
>> > the Connect code instead of referring to "flow" anywhere.
>> > Daniel
>> >
>> > Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún. 5.,
>> H,
>> > 14:49):
>> >
>> > > Hi Chris,
>> > >
>> > > Thank you for your comments!
>> > >
>> > > I agree that the toString based logging is not ideal, and I believe
>> all
>> > > occurrences are within a proper logging context, so they can be
>> ignored.
>> > > If thread names can be changed unconditionally, I agree, using a new
>> MDC
>> > > key is the ideal solution.
>> > >
>> > > Will update the KIP accordingly.
>> > >
>> > > Thanks,
>> > > Daniel
>> > >
>> > > Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún.
>> > 2.,
>> > > P, 22:23):
>> > >
>> > >> Hi Daniel,
>> > >>
>> > >> Are there any cases of Object::toString being used by internal
>> classes
>> > for
>> > >> logging context that can't be covered by MDC contexts? For example,
>> > >> anything logged by WorkerSinkTask (or any WorkerTask subclass)
>> already
>> > has
>> > >> the MDC set for the task [1]. Since the Object::toString-prefixed
>> style
>> > of
>> > >> logging is a bit obsolete after the introduction of MDC contexts it'd
>> > feel
>> > >> a little strange to continue trying to accommodate it, especially if
>> the
>> > >> changes from this KIP are going to be opt-in regardless.
>> > >>
>> > >> As far as thread names go: unlike log statements, I don't believe
>> > changing
>> > >> them requires a KIP, and would be fine with merging a PR for that
>> > without
>> > >> worrying about compatibility.
>> > >>
>> > >> With that in mind, I'm still wondering if we can add a separate MDC
>> key
>> > >> for
>> > >> MM2 replication flows (perhaps "mirror.maker.flow") and
>> unconditionally
>> > >> add
>> > >> that to the MDC contexts for every thread that gets spun up by each
>> > >> DistributedHerder instance that MM2 creates. This would be different
>> > from
>> > >> the draft PR and KIP, which involve altering the content of the
>> existing
>> > >> "connector.context" MDC key. Users would opt in to the change by
>> > altering
>> > >> their Log4j config instead of altering their MM2 config, which
>> matches
>> > >> precedent set with the introduction of the "connector.context" key.
>> > >>
>> > >> Thoughts?
>> > >>
>> > >> [1] -
>> > >>
>> > >>
>> >
>> https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255
>> > >>
>> > >> Cheers,
>> > >>
>> > >> Chris
>> > >>
>> > >> On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <urb.daniel7@gmail.com
>> >
>> > >> wrote:
>> > >>
>> > >> > Hi Chris,
>> > >> >
>> > >> > Thank you for your comments!
>> > >> > 1. This approach is OK for me. I thought that the "sample" configs
>> in
>> > >> the
>> > >> > repo do not fall into the same category as the default of a new
>> > config.
>> > >> > Adding a commented line instead should be ok, and the future
>> opt-out
>> > >> change
>> > >> > sounds good to me.
>> > >> >
>> > >> > 2. Besides the MDC, there are 2 other places where the flow context
>> > >> > information could be added:
>> > >> > - Some of the Connect internal classes use their .toString methods
>> > when
>> > >> > logging (e.g. WorkerSinkTask has a line like this: log.info("{}
>> > >> Executing
>> > >> > sink task", this);). These toString implementations contain the
>> > >> connector
>> > >> > name, and nothing else, so in MM2 dedicated mode, adding the flow
>> > would
>> > >> > make these lines unique.
>> > >> > - Connect worker thread names. Currently, they contain the
>> connector
>> > >> > name/task ID, but to make them unique, the flow should be added in
>> MM2
>> > >> > distributed mode.
>> > >> > In my draft PR, I changed both of these, but for the sake of
>> backward
>> > >> > compatibility, I made the new toString/thread name dependent on the
>> > new,
>> > >> > suggested configuration flag.
>> > >> > If we go with a new MDC key, but we still want to change the
>> toString
>> > >> > methods and the thread names, we still might need an extra flag to
>> > turn
>> > >> on
>> > >> > the new behavior.
>> > >> > AFAIK the toString calls are all made inside a proper logging
>> context,
>> > >> so
>> > >> > changing the toString methods don't add much value. I think that
>> the
>> > >> thread
>> > >> > name changes are useful, giving more context for log lines written
>> > >> outside
>> > >> > of a log context.
>> > >> >
>> > >> > In short, I think that MDC + thread name changes are necessary to
>> make
>> > >> MM2
>> > >> > dedicated logs useful for diagnostics. If we keep both, then maybe
>> > >> having a
>> > >> > single config to control both at the same time is better than
>> having a
>> > >> new
>> > >> > MDC key (configured separately in the log pattern) and a config
>> flag
>> > >> (set
>> > >> > separately in the properties file) for the thread name change.
>> > >> >
>> > >> > Thanks,
>> > >> > Daniel
>> > >> >
>> > >>
>> > >
>> >
>>
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
Hi Chris,

Thank you for your comments! I updated the KIP. I still need to add the
example before/after log lines, will do that soon, but I addressed all the
other points.
1. Added more details on thread renaming under Public Interfaces, removed
pseudo code.
2. Removed the stale header - originally, client.id related changes were
part of the KIP, and I failed removing all leftovers of that version.
3. Threads listed under Public Interfaces with current/proposed names.
4. Added a comment in the connect-log4j.properties, similar to the one
introduced in KIP-449. We don't have a dedicated MM2 log4j config, not sure
if we should introduce it here.
5. Clarified testing section - I think thread names should not be tested
(they never were), but testing will focus on the new MDC context value.

Thanks,
Daniel

Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún. 5.,
H, 16:46):

> Hi Daniel,
>
> Thanks for the updates! A few more thoughts:
>
> 1. The "Proposed Changes" section seems a bit like a pseudocode
> implementation of the KIP. We don't really need this level of detail; most
> of the time, we're just looking for implementation details that don't
> directly affect the user-facing changes proposed in the "Public Interfaces"
> section but are worth mentioning because they (1) demonstrate how the
> user-facing changes are made possible, (2) indirectly affect user-facing
> behavior, or (3) go into more detail (like providing examples) about the
> user-facing behavior. For this KIP, I think examples of user-facing
> behavior (like before/after of thread names and log messages) and possibly
> an official description of the scope of the changes (which threads are
> going to be renamed and/or include the new MDC key, and which aren't?) are
> all that we'd really need in this section; everything else is fairly clear
> IMO. FWIW, the reason we want to discourage going into too much detail with
> KIPs is that it can quickly devolve into code review over mailing list,
> which can hold KIPs up for longer than necessary when the core design
> changes they contain are already basically accepted by everyone.
>
> 2. The "MM2 distributed mode client.id and log change" header seems like
> it
> may no longer be accurate; the contents do not mention any changes to
> client IDs. I might be missing something though; please correct me if I am.
>
> 3. Can you provide some before/after examples of what thread names and log
> messages will look like? I'm wondering about the thread that the
> distributed herder runs on, threads for connectors and tasks, and threads
> for polling internal topics (which we currently manage with the
> KafkaBasedLog class). It's fine if some of these are unchanged, I just want
> to better understand the scope of the proposed changes and get an idea of
> how they may appear to users.
>
> 4. There's no mention of changes to the default Log4j config files that we
> ship. Is this intentional? I feel like we need some way for users to easily
> discover this feature; if we're not going to touch our default Log4j config
> files, is there another way that we can expect users to find out about the
> new MDC key?
>
> 5. RE the "Test Plan" section: can you provide a little more detail of
> which cases we'll be covering with the proposed unit tests? Will we be
> verifying that the MDC context is set in various places? If so, which
> places? And the same with thread names? (There doesn't have to be a ton of
> detail, but a little more than "unit tests" would be nice 😄)
>
> Cheers,
>
> Chris
>
> On Mon, Jun 5, 2023 at 9:45 AM Dániel Urbán <ur...@gmail.com> wrote:
>
> > I updated the KIP accordingly. Tried to come up with more generic terms
> in
> > the Connect code instead of referring to "flow" anywhere.
> > Daniel
> >
> > Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún. 5.,
> H,
> > 14:49):
> >
> > > Hi Chris,
> > >
> > > Thank you for your comments!
> > >
> > > I agree that the toString based logging is not ideal, and I believe all
> > > occurrences are within a proper logging context, so they can be
> ignored.
> > > If thread names can be changed unconditionally, I agree, using a new
> MDC
> > > key is the ideal solution.
> > >
> > > Will update the KIP accordingly.
> > >
> > > Thanks,
> > > Daniel
> > >
> > > Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún.
> > 2.,
> > > P, 22:23):
> > >
> > >> Hi Daniel,
> > >>
> > >> Are there any cases of Object::toString being used by internal classes
> > for
> > >> logging context that can't be covered by MDC contexts? For example,
> > >> anything logged by WorkerSinkTask (or any WorkerTask subclass) already
> > has
> > >> the MDC set for the task [1]. Since the Object::toString-prefixed
> style
> > of
> > >> logging is a bit obsolete after the introduction of MDC contexts it'd
> > feel
> > >> a little strange to continue trying to accommodate it, especially if
> the
> > >> changes from this KIP are going to be opt-in regardless.
> > >>
> > >> As far as thread names go: unlike log statements, I don't believe
> > changing
> > >> them requires a KIP, and would be fine with merging a PR for that
> > without
> > >> worrying about compatibility.
> > >>
> > >> With that in mind, I'm still wondering if we can add a separate MDC
> key
> > >> for
> > >> MM2 replication flows (perhaps "mirror.maker.flow") and
> unconditionally
> > >> add
> > >> that to the MDC contexts for every thread that gets spun up by each
> > >> DistributedHerder instance that MM2 creates. This would be different
> > from
> > >> the draft PR and KIP, which involve altering the content of the
> existing
> > >> "connector.context" MDC key. Users would opt in to the change by
> > altering
> > >> their Log4j config instead of altering their MM2 config, which matches
> > >> precedent set with the introduction of the "connector.context" key.
> > >>
> > >> Thoughts?
> > >>
> > >> [1] -
> > >>
> > >>
> >
> https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255
> > >>
> > >> Cheers,
> > >>
> > >> Chris
> > >>
> > >> On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <ur...@gmail.com>
> > >> wrote:
> > >>
> > >> > Hi Chris,
> > >> >
> > >> > Thank you for your comments!
> > >> > 1. This approach is OK for me. I thought that the "sample" configs
> in
> > >> the
> > >> > repo do not fall into the same category as the default of a new
> > config.
> > >> > Adding a commented line instead should be ok, and the future opt-out
> > >> change
> > >> > sounds good to me.
> > >> >
> > >> > 2. Besides the MDC, there are 2 other places where the flow context
> > >> > information could be added:
> > >> > - Some of the Connect internal classes use their .toString methods
> > when
> > >> > logging (e.g. WorkerSinkTask has a line like this: log.info("{}
> > >> Executing
> > >> > sink task", this);). These toString implementations contain the
> > >> connector
> > >> > name, and nothing else, so in MM2 dedicated mode, adding the flow
> > would
> > >> > make these lines unique.
> > >> > - Connect worker thread names. Currently, they contain the connector
> > >> > name/task ID, but to make them unique, the flow should be added in
> MM2
> > >> > distributed mode.
> > >> > In my draft PR, I changed both of these, but for the sake of
> backward
> > >> > compatibility, I made the new toString/thread name dependent on the
> > new,
> > >> > suggested configuration flag.
> > >> > If we go with a new MDC key, but we still want to change the
> toString
> > >> > methods and the thread names, we still might need an extra flag to
> > turn
> > >> on
> > >> > the new behavior.
> > >> > AFAIK the toString calls are all made inside a proper logging
> context,
> > >> so
> > >> > changing the toString methods don't add much value. I think that the
> > >> thread
> > >> > name changes are useful, giving more context for log lines written
> > >> outside
> > >> > of a log context.
> > >> >
> > >> > In short, I think that MDC + thread name changes are necessary to
> make
> > >> MM2
> > >> > dedicated logs useful for diagnostics. If we keep both, then maybe
> > >> having a
> > >> > single config to control both at the same time is better than
> having a
> > >> new
> > >> > MDC key (configured separately in the log pattern) and a config flag
> > >> (set
> > >> > separately in the properties file) for the thread name change.
> > >> >
> > >> > Thanks,
> > >> > Daniel
> > >> >
> > >>
> > >
> >
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Chris Egerton <ch...@aiven.io.INVALID>.
Hi Daniel,

Thanks for the updates! A few more thoughts:

1. The "Proposed Changes" section seems a bit like a pseudocode
implementation of the KIP. We don't really need this level of detail; most
of the time, we're just looking for implementation details that don't
directly affect the user-facing changes proposed in the "Public Interfaces"
section but are worth mentioning because they (1) demonstrate how the
user-facing changes are made possible, (2) indirectly affect user-facing
behavior, or (3) go into more detail (like providing examples) about the
user-facing behavior. For this KIP, I think examples of user-facing
behavior (like before/after of thread names and log messages) and possibly
an official description of the scope of the changes (which threads are
going to be renamed and/or include the new MDC key, and which aren't?) are
all that we'd really need in this section; everything else is fairly clear
IMO. FWIW, the reason we want to discourage going into too much detail with
KIPs is that it can quickly devolve into code review over mailing list,
which can hold KIPs up for longer than necessary when the core design
changes they contain are already basically accepted by everyone.

2. The "MM2 distributed mode client.id and log change" header seems like it
may no longer be accurate; the contents do not mention any changes to
client IDs. I might be missing something though; please correct me if I am.

3. Can you provide some before/after examples of what thread names and log
messages will look like? I'm wondering about the thread that the
distributed herder runs on, threads for connectors and tasks, and threads
for polling internal topics (which we currently manage with the
KafkaBasedLog class). It's fine if some of these are unchanged, I just want
to better understand the scope of the proposed changes and get an idea of
how they may appear to users.

4. There's no mention of changes to the default Log4j config files that we
ship. Is this intentional? I feel like we need some way for users to easily
discover this feature; if we're not going to touch our default Log4j config
files, is there another way that we can expect users to find out about the
new MDC key?

5. RE the "Test Plan" section: can you provide a little more detail of
which cases we'll be covering with the proposed unit tests? Will we be
verifying that the MDC context is set in various places? If so, which
places? And the same with thread names? (There doesn't have to be a ton of
detail, but a little more than "unit tests" would be nice 😄)

Cheers,

Chris

On Mon, Jun 5, 2023 at 9:45 AM Dániel Urbán <ur...@gmail.com> wrote:

> I updated the KIP accordingly. Tried to come up with more generic terms in
> the Connect code instead of referring to "flow" anywhere.
> Daniel
>
> Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún. 5., H,
> 14:49):
>
> > Hi Chris,
> >
> > Thank you for your comments!
> >
> > I agree that the toString based logging is not ideal, and I believe all
> > occurrences are within a proper logging context, so they can be ignored.
> > If thread names can be changed unconditionally, I agree, using a new MDC
> > key is the ideal solution.
> >
> > Will update the KIP accordingly.
> >
> > Thanks,
> > Daniel
> >
> > Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún.
> 2.,
> > P, 22:23):
> >
> >> Hi Daniel,
> >>
> >> Are there any cases of Object::toString being used by internal classes
> for
> >> logging context that can't be covered by MDC contexts? For example,
> >> anything logged by WorkerSinkTask (or any WorkerTask subclass) already
> has
> >> the MDC set for the task [1]. Since the Object::toString-prefixed style
> of
> >> logging is a bit obsolete after the introduction of MDC contexts it'd
> feel
> >> a little strange to continue trying to accommodate it, especially if the
> >> changes from this KIP are going to be opt-in regardless.
> >>
> >> As far as thread names go: unlike log statements, I don't believe
> changing
> >> them requires a KIP, and would be fine with merging a PR for that
> without
> >> worrying about compatibility.
> >>
> >> With that in mind, I'm still wondering if we can add a separate MDC key
> >> for
> >> MM2 replication flows (perhaps "mirror.maker.flow") and unconditionally
> >> add
> >> that to the MDC contexts for every thread that gets spun up by each
> >> DistributedHerder instance that MM2 creates. This would be different
> from
> >> the draft PR and KIP, which involve altering the content of the existing
> >> "connector.context" MDC key. Users would opt in to the change by
> altering
> >> their Log4j config instead of altering their MM2 config, which matches
> >> precedent set with the introduction of the "connector.context" key.
> >>
> >> Thoughts?
> >>
> >> [1] -
> >>
> >>
> https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255
> >>
> >> Cheers,
> >>
> >> Chris
> >>
> >> On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <ur...@gmail.com>
> >> wrote:
> >>
> >> > Hi Chris,
> >> >
> >> > Thank you for your comments!
> >> > 1. This approach is OK for me. I thought that the "sample" configs in
> >> the
> >> > repo do not fall into the same category as the default of a new
> config.
> >> > Adding a commented line instead should be ok, and the future opt-out
> >> change
> >> > sounds good to me.
> >> >
> >> > 2. Besides the MDC, there are 2 other places where the flow context
> >> > information could be added:
> >> > - Some of the Connect internal classes use their .toString methods
> when
> >> > logging (e.g. WorkerSinkTask has a line like this: log.info("{}
> >> Executing
> >> > sink task", this);). These toString implementations contain the
> >> connector
> >> > name, and nothing else, so in MM2 dedicated mode, adding the flow
> would
> >> > make these lines unique.
> >> > - Connect worker thread names. Currently, they contain the connector
> >> > name/task ID, but to make them unique, the flow should be added in MM2
> >> > distributed mode.
> >> > In my draft PR, I changed both of these, but for the sake of backward
> >> > compatibility, I made the new toString/thread name dependent on the
> new,
> >> > suggested configuration flag.
> >> > If we go with a new MDC key, but we still want to change the toString
> >> > methods and the thread names, we still might need an extra flag to
> turn
> >> on
> >> > the new behavior.
> >> > AFAIK the toString calls are all made inside a proper logging context,
> >> so
> >> > changing the toString methods don't add much value. I think that the
> >> thread
> >> > name changes are useful, giving more context for log lines written
> >> outside
> >> > of a log context.
> >> >
> >> > In short, I think that MDC + thread name changes are necessary to make
> >> MM2
> >> > dedicated logs useful for diagnostics. If we keep both, then maybe
> >> having a
> >> > single config to control both at the same time is better than having a
> >> new
> >> > MDC key (configured separately in the log pattern) and a config flag
> >> (set
> >> > separately in the properties file) for the thread name change.
> >> >
> >> > Thanks,
> >> > Daniel
> >> >
> >>
> >
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
I updated the KIP accordingly. Tried to come up with more generic terms in
the Connect code instead of referring to "flow" anywhere.
Daniel

Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. jún. 5., H,
14:49):

> Hi Chris,
>
> Thank you for your comments!
>
> I agree that the toString based logging is not ideal, and I believe all
> occurrences are within a proper logging context, so they can be ignored.
> If thread names can be changed unconditionally, I agree, using a new MDC
> key is the ideal solution.
>
> Will update the KIP accordingly.
>
> Thanks,
> Daniel
>
> Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún. 2.,
> P, 22:23):
>
>> Hi Daniel,
>>
>> Are there any cases of Object::toString being used by internal classes for
>> logging context that can't be covered by MDC contexts? For example,
>> anything logged by WorkerSinkTask (or any WorkerTask subclass) already has
>> the MDC set for the task [1]. Since the Object::toString-prefixed style of
>> logging is a bit obsolete after the introduction of MDC contexts it'd feel
>> a little strange to continue trying to accommodate it, especially if the
>> changes from this KIP are going to be opt-in regardless.
>>
>> As far as thread names go: unlike log statements, I don't believe changing
>> them requires a KIP, and would be fine with merging a PR for that without
>> worrying about compatibility.
>>
>> With that in mind, I'm still wondering if we can add a separate MDC key
>> for
>> MM2 replication flows (perhaps "mirror.maker.flow") and unconditionally
>> add
>> that to the MDC contexts for every thread that gets spun up by each
>> DistributedHerder instance that MM2 creates. This would be different from
>> the draft PR and KIP, which involve altering the content of the existing
>> "connector.context" MDC key. Users would opt in to the change by altering
>> their Log4j config instead of altering their MM2 config, which matches
>> precedent set with the introduction of the "connector.context" key.
>>
>> Thoughts?
>>
>> [1] -
>>
>> https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255
>>
>> Cheers,
>>
>> Chris
>>
>> On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <ur...@gmail.com>
>> wrote:
>>
>> > Hi Chris,
>> >
>> > Thank you for your comments!
>> > 1. This approach is OK for me. I thought that the "sample" configs in
>> the
>> > repo do not fall into the same category as the default of a new config.
>> > Adding a commented line instead should be ok, and the future opt-out
>> change
>> > sounds good to me.
>> >
>> > 2. Besides the MDC, there are 2 other places where the flow context
>> > information could be added:
>> > - Some of the Connect internal classes use their .toString methods when
>> > logging (e.g. WorkerSinkTask has a line like this: log.info("{}
>> Executing
>> > sink task", this);). These toString implementations contain the
>> connector
>> > name, and nothing else, so in MM2 dedicated mode, adding the flow would
>> > make these lines unique.
>> > - Connect worker thread names. Currently, they contain the connector
>> > name/task ID, but to make them unique, the flow should be added in MM2
>> > distributed mode.
>> > In my draft PR, I changed both of these, but for the sake of backward
>> > compatibility, I made the new toString/thread name dependent on the new,
>> > suggested configuration flag.
>> > If we go with a new MDC key, but we still want to change the toString
>> > methods and the thread names, we still might need an extra flag to turn
>> on
>> > the new behavior.
>> > AFAIK the toString calls are all made inside a proper logging context,
>> so
>> > changing the toString methods don't add much value. I think that the
>> thread
>> > name changes are useful, giving more context for log lines written
>> outside
>> > of a log context.
>> >
>> > In short, I think that MDC + thread name changes are necessary to make
>> MM2
>> > dedicated logs useful for diagnostics. If we keep both, then maybe
>> having a
>> > single config to control both at the same time is better than having a
>> new
>> > MDC key (configured separately in the log pattern) and a config flag
>> (set
>> > separately in the properties file) for the thread name change.
>> >
>> > Thanks,
>> > Daniel
>> >
>>
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
Hi Chris,

Thank you for your comments!

I agree that the toString based logging is not ideal, and I believe all
occurrences are within a proper logging context, so they can be ignored.
If thread names can be changed unconditionally, I agree, using a new MDC
key is the ideal solution.

Will update the KIP accordingly.

Thanks,
Daniel

Chris Egerton <ch...@aiven.io.invalid> ezt írta (időpont: 2023. jún. 2.,
P, 22:23):

> Hi Daniel,
>
> Are there any cases of Object::toString being used by internal classes for
> logging context that can't be covered by MDC contexts? For example,
> anything logged by WorkerSinkTask (or any WorkerTask subclass) already has
> the MDC set for the task [1]. Since the Object::toString-prefixed style of
> logging is a bit obsolete after the introduction of MDC contexts it'd feel
> a little strange to continue trying to accommodate it, especially if the
> changes from this KIP are going to be opt-in regardless.
>
> As far as thread names go: unlike log statements, I don't believe changing
> them requires a KIP, and would be fine with merging a PR for that without
> worrying about compatibility.
>
> With that in mind, I'm still wondering if we can add a separate MDC key for
> MM2 replication flows (perhaps "mirror.maker.flow") and unconditionally add
> that to the MDC contexts for every thread that gets spun up by each
> DistributedHerder instance that MM2 creates. This would be different from
> the draft PR and KIP, which involve altering the content of the existing
> "connector.context" MDC key. Users would opt in to the change by altering
> their Log4j config instead of altering their MM2 config, which matches
> precedent set with the introduction of the "connector.context" key.
>
> Thoughts?
>
> [1] -
>
> https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255
>
> Cheers,
>
> Chris
>
> On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <ur...@gmail.com>
> wrote:
>
> > Hi Chris,
> >
> > Thank you for your comments!
> > 1. This approach is OK for me. I thought that the "sample" configs in the
> > repo do not fall into the same category as the default of a new config.
> > Adding a commented line instead should be ok, and the future opt-out
> change
> > sounds good to me.
> >
> > 2. Besides the MDC, there are 2 other places where the flow context
> > information could be added:
> > - Some of the Connect internal classes use their .toString methods when
> > logging (e.g. WorkerSinkTask has a line like this: log.info("{}
> Executing
> > sink task", this);). These toString implementations contain the connector
> > name, and nothing else, so in MM2 dedicated mode, adding the flow would
> > make these lines unique.
> > - Connect worker thread names. Currently, they contain the connector
> > name/task ID, but to make them unique, the flow should be added in MM2
> > distributed mode.
> > In my draft PR, I changed both of these, but for the sake of backward
> > compatibility, I made the new toString/thread name dependent on the new,
> > suggested configuration flag.
> > If we go with a new MDC key, but we still want to change the toString
> > methods and the thread names, we still might need an extra flag to turn
> on
> > the new behavior.
> > AFAIK the toString calls are all made inside a proper logging context, so
> > changing the toString methods don't add much value. I think that the
> thread
> > name changes are useful, giving more context for log lines written
> outside
> > of a log context.
> >
> > In short, I think that MDC + thread name changes are necessary to make
> MM2
> > dedicated logs useful for diagnostics. If we keep both, then maybe
> having a
> > single config to control both at the same time is better than having a
> new
> > MDC key (configured separately in the log pattern) and a config flag (set
> > separately in the properties file) for the thread name change.
> >
> > Thanks,
> > Daniel
> >
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Chris Egerton <ch...@aiven.io.INVALID>.
Hi Daniel,

Are there any cases of Object::toString being used by internal classes for
logging context that can't be covered by MDC contexts? For example,
anything logged by WorkerSinkTask (or any WorkerTask subclass) already has
the MDC set for the task [1]. Since the Object::toString-prefixed style of
logging is a bit obsolete after the introduction of MDC contexts it'd feel
a little strange to continue trying to accommodate it, especially if the
changes from this KIP are going to be opt-in regardless.

As far as thread names go: unlike log statements, I don't believe changing
them requires a KIP, and would be fine with merging a PR for that without
worrying about compatibility.

With that in mind, I'm still wondering if we can add a separate MDC key for
MM2 replication flows (perhaps "mirror.maker.flow") and unconditionally add
that to the MDC contexts for every thread that gets spun up by each
DistributedHerder instance that MM2 creates. This would be different from
the draft PR and KIP, which involve altering the content of the existing
"connector.context" MDC key. Users would opt in to the change by altering
their Log4j config instead of altering their MM2 config, which matches
precedent set with the introduction of the "connector.context" key.

Thoughts?

[1] -
https://github.com/apache/kafka/blob/146a6976aed0d9f90c70b6f21dca8b887cc34e71/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java#L255

Cheers,

Chris

On Tue, May 23, 2023 at 11:36 AM Dániel Urbán <ur...@gmail.com> wrote:

> Hi Chris,
>
> Thank you for your comments!
> 1. This approach is OK for me. I thought that the "sample" configs in the
> repo do not fall into the same category as the default of a new config.
> Adding a commented line instead should be ok, and the future opt-out change
> sounds good to me.
>
> 2. Besides the MDC, there are 2 other places where the flow context
> information could be added:
> - Some of the Connect internal classes use their .toString methods when
> logging (e.g. WorkerSinkTask has a line like this: log.info("{} Executing
> sink task", this);). These toString implementations contain the connector
> name, and nothing else, so in MM2 dedicated mode, adding the flow would
> make these lines unique.
> - Connect worker thread names. Currently, they contain the connector
> name/task ID, but to make them unique, the flow should be added in MM2
> distributed mode.
> In my draft PR, I changed both of these, but for the sake of backward
> compatibility, I made the new toString/thread name dependent on the new,
> suggested configuration flag.
> If we go with a new MDC key, but we still want to change the toString
> methods and the thread names, we still might need an extra flag to turn on
> the new behavior.
> AFAIK the toString calls are all made inside a proper logging context, so
> changing the toString methods don't add much value. I think that the thread
> name changes are useful, giving more context for log lines written outside
> of a log context.
>
> In short, I think that MDC + thread name changes are necessary to make MM2
> dedicated logs useful for diagnostics. If we keep both, then maybe having a
> single config to control both at the same time is better than having a new
> MDC key (configured separately in the log pattern) and a config flag (set
> separately in the properties file) for the thread name change.
>
> Thanks,
> Daniel
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
Hi Chris,

Thank you for your comments!
1. This approach is OK for me. I thought that the "sample" configs in the
repo do not fall into the same category as the default of a new config.
Adding a commented line instead should be ok, and the future opt-out change
sounds good to me.

2. Besides the MDC, there are 2 other places where the flow context
information could be added:
- Some of the Connect internal classes use their .toString methods when
logging (e.g. WorkerSinkTask has a line like this: log.info("{} Executing
sink task", this);). These toString implementations contain the connector
name, and nothing else, so in MM2 dedicated mode, adding the flow would
make these lines unique.
- Connect worker thread names. Currently, they contain the connector
name/task ID, but to make them unique, the flow should be added in MM2
distributed mode.
In my draft PR, I changed both of these, but for the sake of backward
compatibility, I made the new toString/thread name dependent on the new,
suggested configuration flag.
If we go with a new MDC key, but we still want to change the toString
methods and the thread names, we still might need an extra flag to turn on
the new behavior.
AFAIK the toString calls are all made inside a proper logging context, so
changing the toString methods don't add much value. I think that the thread
name changes are useful, giving more context for log lines written outside
of a log context.

In short, I think that MDC + thread name changes are necessary to make MM2
dedicated logs useful for diagnostics. If we keep both, then maybe having a
single config to control both at the same time is better than having a new
MDC key (configured separately in the log pattern) and a config flag (set
separately in the properties file) for the thread name change.

Thanks,
Daniel

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Chris Egerton <ch...@aiven.io.INVALID>.
Hi Daniel,

Thanks for the KIP! A few thoughts:

1. This change is similar to when we added connector contexts to Connect in
KIPs 449 [1] and 721 [2]. I wonder if we can follow a similar approach
(combining both KIPs' changes into this single one), by doing the following:
- Disabling the changes in the log context by default, both
programmatically (which is already proposed in the KIP) and in any relevant
config files (not specified in the KIP, but contrary to the latest commit
in the draft PR for this change)
- Add a commented-out line to all relevant config files that enables the
log context changes, with an explanation above it of what un-commenting the
line does, that it's recommended for users bringing up new clusters to
un-comment that line, and that...
- We'll change the default for this behavior to be opt-out instead of
opt-in in the next major release (probably 4.0.0 unless this KIP takes a
while to be accepted), at which point we can also either remove the
commented-out lines from all relevant config files, or change them to
demonstrate how to disable the new log context behavior and change the
comment to explain that the line below can be un-commented in order to
restore prior behavior

2. Can you shed a little more light on why we don't want to do this with a
new MDC context key? Seems like it might be a bit easier to implement now,
less brittle for future changes, and easier for users to grok if they're
already familiar with the connector log context MDC key.

Cheers,

Chris

[1] -
https://cwiki.apache.org/confluence/display/KAFKA/KIP-449%3A+Add+connector+contexts+to+Connect+worker+logs
[2] -
https://cwiki.apache.org/confluence/display/KAFKA/KIP-721%3A+Enable+connector+log+contexts+in+Connect+Log4j+configuration

On Thu, May 11, 2023 at 7:06 AM Dániel Urbán <ur...@gmail.com> wrote:

> Hello everyone,
> I would like to bump this thread, pretty straightforward KIP, and helps a
> lot with MM2 dedicated mode diagnostics.
> Thanks in advance,
> Daniel
>
> Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. máj. 4., Cs,
> 14:08):
>
> > Hi Viktor,
> >
> > Thank you for your comments. I agree that this change is low-risk - the
> > default false feature flag shouldn't cause any problems to existing
> users.
> >
> > As for the rejected alternative of adding an attribute to the MDC - some
> > of the internal worker classes (such as the different WorkerTask
> > subclasses) have a toString implementation which is used in the log
> > message. When those toString calls are used in logging, we don't always
> > have a logging context with MDC attributes. In my current PR, I changed
> > those toString methods in a backward compatible way, so if the feature
> flag
> > is set to false, the method will return the same string as it used to.
> Even
> > if we went with the extra MDC attribute, we would still probably need an
> > extra config to make the toString methods backward compatible. Because of
> > this, it might be easier to have a single flag to control the full
> feature.
> >
> > Daniel
> >
> > Viktor Somogyi-Vass <vi...@cloudera.com.invalid> ezt írta
> > (időpont: 2023. ápr. 21., P, 15:07):
> >
> >> Hi Daniel,
> >>
> >> I think this is a useful addition, it helps resolving issues and
> >> escalations, and improves overall traceability.
> >> Changing the logging context may imply the risk of making certain log
> >> parsers unable to work on new logs. As I see we by default disable this
> >> feature which solves this problem, however I also think that by
> disabling
> >> it by default it isn't much of a help because users may not know about
> >> this
> >> configuration and would not benefit from these when they face problems.
> So
> >> overall I'd like to go with default=true and wanted to put this out here
> >> for the community to discuss whether it's a problem.
> >> Also, what was the reasoning behind rejecting the second alternative?
> As I
> >> see that would be a viable option and maybe a bit more idiomatic to the
> >> logging framework.
> >>
> >> A minor note: please update the JIRA link in the KIP to point to the
> right
> >> one.
> >>
> >> Best,
> >> Viktor
> >>
> >> On Thu, Apr 13, 2023 at 2:19 PM Dániel Urbán <ur...@gmail.com>
> >> wrote:
> >>
> >> > Hi everyone,
> >> >
> >> > I would like to bump this thread. I think this would be very useful
> for
> >> any
> >> > MM2 users, as the current logs with certain architectures (e.g.
> fan-out)
> >> > are impossible to decipher.
> >> > I already submitted a PR to demonstrate the proposed solution:
> >> > https://github.com/apache/kafka/pull/13475
> >> >
> >> > Thanks for your comments in advance,
> >> > Daniel
> >> >
> >> > Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. márc.
> >> 30.,
> >> > Cs, 18:24):
> >> >
> >> > > Hello everyone,
> >> > >
> >> > > I would like to kick off a discussion about KIP-916:
> >> > >
> >> >
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-916%3A+MM2+distributed+mode+flow+log+context
> >> > >
> >> > > The KIP aims to enhance the diagnostic information for MM2
> distributed
> >> > > mode. MM2 relies on multiple Connect worker instances nested in a
> >> single
> >> > > process. In Connect, Connector names are guaranteed to be unique in
> a
> >> > > single process, but in MM2, this is not true. Because of this, the
> >> > > diagnostics provided by Connect (client.ids, log context) do not
> >> ensure
> >> > > that logs are distinguishable for different flows (Connect workers)
> >> > inside
> >> > > an MM2 process.
> >> > >
> >> > > Thanks for all you input in advance,
> >> > > Daniel
> >> > >
> >> >
> >>
> >
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
Hello everyone,
I would like to bump this thread, pretty straightforward KIP, and helps a
lot with MM2 dedicated mode diagnostics.
Thanks in advance,
Daniel

Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. máj. 4., Cs,
14:08):

> Hi Viktor,
>
> Thank you for your comments. I agree that this change is low-risk - the
> default false feature flag shouldn't cause any problems to existing users.
>
> As for the rejected alternative of adding an attribute to the MDC - some
> of the internal worker classes (such as the different WorkerTask
> subclasses) have a toString implementation which is used in the log
> message. When those toString calls are used in logging, we don't always
> have a logging context with MDC attributes. In my current PR, I changed
> those toString methods in a backward compatible way, so if the feature flag
> is set to false, the method will return the same string as it used to. Even
> if we went with the extra MDC attribute, we would still probably need an
> extra config to make the toString methods backward compatible. Because of
> this, it might be easier to have a single flag to control the full feature.
>
> Daniel
>
> Viktor Somogyi-Vass <vi...@cloudera.com.invalid> ezt írta
> (időpont: 2023. ápr. 21., P, 15:07):
>
>> Hi Daniel,
>>
>> I think this is a useful addition, it helps resolving issues and
>> escalations, and improves overall traceability.
>> Changing the logging context may imply the risk of making certain log
>> parsers unable to work on new logs. As I see we by default disable this
>> feature which solves this problem, however I also think that by disabling
>> it by default it isn't much of a help because users may not know about
>> this
>> configuration and would not benefit from these when they face problems. So
>> overall I'd like to go with default=true and wanted to put this out here
>> for the community to discuss whether it's a problem.
>> Also, what was the reasoning behind rejecting the second alternative? As I
>> see that would be a viable option and maybe a bit more idiomatic to the
>> logging framework.
>>
>> A minor note: please update the JIRA link in the KIP to point to the right
>> one.
>>
>> Best,
>> Viktor
>>
>> On Thu, Apr 13, 2023 at 2:19 PM Dániel Urbán <ur...@gmail.com>
>> wrote:
>>
>> > Hi everyone,
>> >
>> > I would like to bump this thread. I think this would be very useful for
>> any
>> > MM2 users, as the current logs with certain architectures (e.g. fan-out)
>> > are impossible to decipher.
>> > I already submitted a PR to demonstrate the proposed solution:
>> > https://github.com/apache/kafka/pull/13475
>> >
>> > Thanks for your comments in advance,
>> > Daniel
>> >
>> > Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. márc.
>> 30.,
>> > Cs, 18:24):
>> >
>> > > Hello everyone,
>> > >
>> > > I would like to kick off a discussion about KIP-916:
>> > >
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-916%3A+MM2+distributed+mode+flow+log+context
>> > >
>> > > The KIP aims to enhance the diagnostic information for MM2 distributed
>> > > mode. MM2 relies on multiple Connect worker instances nested in a
>> single
>> > > process. In Connect, Connector names are guaranteed to be unique in a
>> > > single process, but in MM2, this is not true. Because of this, the
>> > > diagnostics provided by Connect (client.ids, log context) do not
>> ensure
>> > > that logs are distinguishable for different flows (Connect workers)
>> > inside
>> > > an MM2 process.
>> > >
>> > > Thanks for all you input in advance,
>> > > Daniel
>> > >
>> >
>>
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
Hi Viktor,

Thank you for your comments. I agree that this change is low-risk - the
default false feature flag shouldn't cause any problems to existing users.

As for the rejected alternative of adding an attribute to the MDC - some of
the internal worker classes (such as the different WorkerTask subclasses)
have a toString implementation which is used in the log message. When those
toString calls are used in logging, we don't always have a logging context
with MDC attributes. In my current PR, I changed those toString methods in
a backward compatible way, so if the feature flag is set to false, the
method will return the same string as it used to. Even if we went with the
extra MDC attribute, we would still probably need an extra config to make
the toString methods backward compatible. Because of this, it might be
easier to have a single flag to control the full feature.

Daniel

Viktor Somogyi-Vass <vi...@cloudera.com.invalid> ezt írta
(időpont: 2023. ápr. 21., P, 15:07):

> Hi Daniel,
>
> I think this is a useful addition, it helps resolving issues and
> escalations, and improves overall traceability.
> Changing the logging context may imply the risk of making certain log
> parsers unable to work on new logs. As I see we by default disable this
> feature which solves this problem, however I also think that by disabling
> it by default it isn't much of a help because users may not know about this
> configuration and would not benefit from these when they face problems. So
> overall I'd like to go with default=true and wanted to put this out here
> for the community to discuss whether it's a problem.
> Also, what was the reasoning behind rejecting the second alternative? As I
> see that would be a viable option and maybe a bit more idiomatic to the
> logging framework.
>
> A minor note: please update the JIRA link in the KIP to point to the right
> one.
>
> Best,
> Viktor
>
> On Thu, Apr 13, 2023 at 2:19 PM Dániel Urbán <ur...@gmail.com>
> wrote:
>
> > Hi everyone,
> >
> > I would like to bump this thread. I think this would be very useful for
> any
> > MM2 users, as the current logs with certain architectures (e.g. fan-out)
> > are impossible to decipher.
> > I already submitted a PR to demonstrate the proposed solution:
> > https://github.com/apache/kafka/pull/13475
> >
> > Thanks for your comments in advance,
> > Daniel
> >
> > Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. márc. 30.,
> > Cs, 18:24):
> >
> > > Hello everyone,
> > >
> > > I would like to kick off a discussion about KIP-916:
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-916%3A+MM2+distributed+mode+flow+log+context
> > >
> > > The KIP aims to enhance the diagnostic information for MM2 distributed
> > > mode. MM2 relies on multiple Connect worker instances nested in a
> single
> > > process. In Connect, Connector names are guaranteed to be unique in a
> > > single process, but in MM2, this is not true. Because of this, the
> > > diagnostics provided by Connect (client.ids, log context) do not ensure
> > > that logs are distinguishable for different flows (Connect workers)
> > inside
> > > an MM2 process.
> > >
> > > Thanks for all you input in advance,
> > > Daniel
> > >
> >
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Viktor Somogyi-Vass <vi...@cloudera.com.INVALID>.
Hi Daniel,

I think this is a useful addition, it helps resolving issues and
escalations, and improves overall traceability.
Changing the logging context may imply the risk of making certain log
parsers unable to work on new logs. As I see we by default disable this
feature which solves this problem, however I also think that by disabling
it by default it isn't much of a help because users may not know about this
configuration and would not benefit from these when they face problems. So
overall I'd like to go with default=true and wanted to put this out here
for the community to discuss whether it's a problem.
Also, what was the reasoning behind rejecting the second alternative? As I
see that would be a viable option and maybe a bit more idiomatic to the
logging framework.

A minor note: please update the JIRA link in the KIP to point to the right
one.

Best,
Viktor

On Thu, Apr 13, 2023 at 2:19 PM Dániel Urbán <ur...@gmail.com> wrote:

> Hi everyone,
>
> I would like to bump this thread. I think this would be very useful for any
> MM2 users, as the current logs with certain architectures (e.g. fan-out)
> are impossible to decipher.
> I already submitted a PR to demonstrate the proposed solution:
> https://github.com/apache/kafka/pull/13475
>
> Thanks for your comments in advance,
> Daniel
>
> Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. márc. 30.,
> Cs, 18:24):
>
> > Hello everyone,
> >
> > I would like to kick off a discussion about KIP-916:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-916%3A+MM2+distributed+mode+flow+log+context
> >
> > The KIP aims to enhance the diagnostic information for MM2 distributed
> > mode. MM2 relies on multiple Connect worker instances nested in a single
> > process. In Connect, Connector names are guaranteed to be unique in a
> > single process, but in MM2, this is not true. Because of this, the
> > diagnostics provided by Connect (client.ids, log context) do not ensure
> > that logs are distinguishable for different flows (Connect workers)
> inside
> > an MM2 process.
> >
> > Thanks for all you input in advance,
> > Daniel
> >
>

Re: [DISCUSS] KIP-916: MM2 distributed mode flow log context

Posted by Dániel Urbán <ur...@gmail.com>.
Hi everyone,

I would like to bump this thread. I think this would be very useful for any
MM2 users, as the current logs with certain architectures (e.g. fan-out)
are impossible to decipher.
I already submitted a PR to demonstrate the proposed solution:
https://github.com/apache/kafka/pull/13475

Thanks for your comments in advance,
Daniel

Dániel Urbán <ur...@gmail.com> ezt írta (időpont: 2023. márc. 30.,
Cs, 18:24):

> Hello everyone,
>
> I would like to kick off a discussion about KIP-916:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-916%3A+MM2+distributed+mode+flow+log+context
>
> The KIP aims to enhance the diagnostic information for MM2 distributed
> mode. MM2 relies on multiple Connect worker instances nested in a single
> process. In Connect, Connector names are guaranteed to be unique in a
> single process, but in MM2, this is not true. Because of this, the
> diagnostics provided by Connect (client.ids, log context) do not ensure
> that logs are distinguishable for different flows (Connect workers) inside
> an MM2 process.
>
> Thanks for all you input in advance,
> Daniel
>