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 <urb.dani...@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 <urb.dani...@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 <viktor.somo...@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 <urb.dani...@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 <urb.dani...@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 > >> > > > >> > > >> > > >
