🐛 bug fix: make sure logs from threads in replication workers are added…

[cgardens]

… to log file

closes #3824

What

• Checkpointing introduced 2 threads to the ReplicationWorker that are executed via an ExecutorService. We rely on metadata in the MDC to make sure that log lines are appended to our log file. Because in java the MDC of the parent thread is not by default passed to threads executed in an ExecutorService, STDOUT of the newly introduced threads was not making it into the log file.
• To be clear we were logging to STDOUT, we just were not getting these logs into the log file. This was particularly bad because when helping users debug issues, we generally ask them to download the scheduler logs from the UI. These logs come directly from the log file. So these important logs were not included in the debugging information, making it very hard to debug issues (and was also a red herring making it look like certain parts of the code path were not running when really they were).

How

• Explicitly copy the MDC of the parent thread into the children threads.
• Add test to verify that the logging works as expected.

[michel-tricot]

WHat are the good practices for the mgmt of MDC? the current handling looks brittle.

[davinchia]

This is tricky!

For discussion sake: maybe there should be an airbyte-commons function that we only use to create callables/runnables? this common function can also make sure MDC is set.

[davinchia]

I'm merging this in since I'm also dealing with some logging issues while working on Kube and I want to get all the bug fixes in to stabilise things.

[cgardens]

@michel-tricot this is a good question. I agree the current approach is brittle. I've hunted around a little bit and I'm not finding a solution that I love. Some of the sources that I've ready. SO, article, article. I think the option described in the last article is the one I dislike least (and is closest to what @davinchia suggested as well). Essentially we can create a static factory helper in airbyte commons that passes the MDC from parent to child.

[cgardens]

Actually I experimented with that implementation and I think it's a terrible idea. The issue is that if you ever run the Runnable in the parent thread (e.g. () -> {}.run(), then it will wipe the MDC in the "parent" context. I think this is a pretty obvious foot gun. I will look more closely at how to make an executor service that handles passing the MDC behavior instead.