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
