[FLINK-40208] Add JobMdcRegistry for config-driven MDC enrichment - #28855
Conversation
Introduce JobMdcRegistry, a process-wide registry mapping JobID to an enriched MDC context built from job configuration. Add MdcOptions with the mdc.job-configuration-to-mdc-keys config option (@PublicEvolving). Wire in mdc enrichment on the job/task submission paths. Generated-by: Claude Code
Generated-by: Claude Code (cherry picked from commit ef6633f)
|
@rkhachatryan, thank you for the review before. This time CI is green |
|
Merged. |
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review (v0.21.2)
| title: "Logging Context (MDC)" | ||
| weight: 7 | ||
| type: docs |
There was a problem hiding this comment.
[Suggestion] Hugo weight: 7 collides with sibling docs/content/docs/ops/events.md (also weight: 7), making their nav ordering non-deterministic. — Concrete cost: Hugo falls back to alphabetical tie-breaking between "Events" and "Logging Context (MDC)", so the sidebar position becomes an accident of the title rather than an editorial choice. The same collision exists in docs/content.zh/docs/ops/logging_context.md.
| title: "Logging Context (MDC)" | |
| weight: 7 | |
| type: docs | |
| title: "Logging Context (MDC)" | |
| weight: 8 | |
| type: docs |
— qwen3.8-max-preview via Qwen Code /review (v0.21.2)
There was a problem hiding this comment.
This looks minor, but I might include it if I do any more cleanup
| for (Map.Entry<String, String> entry : mdcKeyMapping.entrySet()) { | ||
| final String value = jobConfiguration.getString(entry.getKey(), null); | ||
| if (value != null && !value.isBlank()) { | ||
| context.put(entry.getValue(), value); |
There was a problem hiding this comment.
[Suggestion] The mapping target (MDC key name, entry.getValue()) is not validated for blank/empty, while the mapping source value is explicitly checked for blankness — an asymmetry that allows an empty or whitespace-only MDC key to be published. — Failure scenario: an operator configures mdc.job-configuration-to-mdc-keys: {my.key: ""} in config.yaml. The code puts an entry with an empty-string key into the context map, which is then set on every thread's MDC for that job. In Log4j2 JSON layout this produces "": "some-value" in every log record; in pattern layout, %X{} with an empty key has implementation-dependent behavior.
| for (Map.Entry<String, String> entry : mdcKeyMapping.entrySet()) { | |
| final String value = jobConfiguration.getString(entry.getKey(), null); | |
| if (value != null && !value.isBlank()) { | |
| context.put(entry.getValue(), value); | |
| for (Map.Entry<String, String> entry : mdcKeyMapping.entrySet()) { | |
| final String value = jobConfiguration.getString(entry.getKey(), null); | |
| final String mdcKey = entry.getValue(); | |
| if (value != null && !value.isBlank() && mdcKey != null && !mdcKey.isBlank()) { | |
| context.put(mdcKey, value); |
— qwen3.8-max-preview via Qwen Code /review (v0.21.2)
There was a problem hiding this comment.
The asymmetry here comes from the fact that key maybe empty until upstream service submitting the job is not providing the value in config. Key side acts as a "switch" for configs that we might expect. Empty value though would be a deliberate misconfiguration. So I'd not complicate the code here with extra check
| public static void registerOrClear(final JobID jobID, final Configuration jobConfiguration) { | ||
| final Map<String, String> context = MdcUtils.asContextData(jobID, jobConfiguration); | ||
| if (context.size() > 1) { | ||
| REGISTRY.put(jobID, context); | ||
| } else { | ||
| unregister(jobID); | ||
| } |
There was a problem hiding this comment.
[Suggestion] No diagnostic logging at any level when MDC enrichment is configured but resolves to zero entries, making misconfiguration invisible at runtime. — Failure scenario: an operator configures mdc.job-configuration-to-mdc-keys: pipeline.name: pipeline-name but the job is submitted without -Dpipeline.name=.... Every mapped key is silently skipped, registerOrClear calls unregister, and all log records carry only flink-job-id. At 3 AM, the oncall engineer sees no error, no warning, and no debug message explaining why the expected pipeline-name field is absent from every log record.
Consider adding a debug-level log that reports the configured mapping size and the resolved entry count, e.g.:
if (LOG.isDebugEnabled()) {
LOG.debug("MDC enrichment for job {}: {} configured key(s), {} resolved entry(ies).",
jobID, mdcKeyMapping.size(), context.size() - 1);
}— qwen3.8-max-preview via Qwen Code /review (v0.21.2)
There was a problem hiding this comment.
Debug logs are normally not enabled in prod. Presence of the field is a signal on its own to confirm whether the feature is working. More realistic scenario is an alert setup for "missing" field and oncall engineer checking what has changed that led to field disappearing (and end to end tests, to catch such regressions before prod)
| JobInformation jobInformation = null; | ||
| try { | ||
| jobInformation = tdd.getJobInformation(); | ||
| } catch (IllegalStateException ignored) { | ||
| // Expected when job information is offloaded to blob storage and not yet loaded. |
There was a problem hiding this comment.
[Suggestion] The new early-deserialization fallback for offloaded job information (the IllegalStateException catch and the jobInformation == null ternary) has no test coverage. — Failure scenario: TaskExecutorSubmissionTest.testJobMdcContextRegisteredOnSubmitTask always constructs a TDD with inline job information, so tdd.getJobInformation() never throws IllegalStateException. If a future refactor removes the catch block or the null-guarded ternary, tasks with job information offloaded to blob storage (the production path for large job graphs) would fail submission with an unhandled IllegalStateException, and no test would catch the regression.
— qwen3.8-max-preview via Qwen Code /review (v0.21.2)
| JobMdcRegistry.registerOrClear(jobId, recoveredJob.getJobConfiguration()); | ||
| try (MdcCloseable ignored = MdcUtils.withContext(MdcUtils.asContextData(jobId))) { |
There was a problem hiding this comment.
[Suggestion] The Dispatcher recovery path (recoverJob) MDC registration and its failure-path cleanup (unregister in the catch block) have no test coverage; DispatcherTest only tests the submission path. — Failure scenario: if the registerOrClear call in recoverJob were removed during a refactor, jobs recovered after a JobManager failover would silently lose enriched MDC context — all log lines for recovered jobs would carry only flink-job-id despite the job having MDC mappings configured. No existing test would fail.
— qwen3.8-max-preview via Qwen Code /review (v0.21.2)
| fileMergingManager.releaseMergingSnapshotManagerForJob(jobId); | ||
| JobMdcRegistry.unregister(jobId); |
There was a problem hiding this comment.
[Suggestion] The TaskExecutor-side cleanup (JobMdcRegistry.unregister in the job release method) has no test; TaskExecutorSubmissionTest only verifies registration, not deregistration on release. — Failure scenario: if the unregister call were removed, every completed job's MDC entry would remain in the static ConcurrentHashMap for the lifetime of the TaskManager process. On a long-running TaskManager processing hundreds of short-lived jobs, this is an unbounded memory leak in a process-wide static registry. The Dispatcher-side cleanup is tested (testJobMdcContextRegisteredOnSubmissionAndClearedOnTermination), but the TM-side equivalent is not.
— qwen3.8-max-preview via Qwen Code /review (v0.21.2)
What is the purpose of the change
Add
JobMdcRegistry, a process-wide registry that mapsJobIDto an enriched MDC context derived from job configuration. This enables operators to surface custom job config values (e.g. org ID, environment) in Flink's JVM logs via MDC.Brief change log
MdcOptionswithmdc.job-configuration-to-mdc-keys(Map<String,String>) config optionJobMdcRegistry: static registry populated inDispatcherandTaskExecutorMdcUtils.asContextData(JobID)to consult the registry before falling back to the plain job-id singletonJobIDLoggingITCase.testEnrichedMdcLoggingto verify enriched keys appear in log outputVerifying this change
This change added tests and can be verified as follows:
JobMdcRegistryTestcovering register, overwrite, unregister, and isolation between jobsMdcUtilsTestwith parameterized tests for config extraction and registry-first lookupJobIDLoggingITCase.testEnrichedMdcLoggingas an end-to-end integration testDoes this pull request potentially affect one of the following parts:
@Public(Evolving): yes (MdcOptionsis@PublicEvolving)Documentation
ConfigOptionWas generative AI tooling used to co-author this PR?
Generated-by: Claude Code