Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Valid Log4j2 patterns can separate correlation fields from their message or duplicate existing MDC values. Legacy asynchronous Log4j2 events also bypass service enrichment, leaving the newly inserted service field empty.
🤖 Bits Code Review · Commit 74d4ab9
| if (c == quote) { | ||
| quote = 0; | ||
| } | ||
| } else if (c == '\'' || c == '"') { |
There was a problem hiding this comment.
With injection enabled, the valid Log4j2 pattern %replace{%m}{'}{}%n acquires correlation fields after its newline. This scanner incorrectly treats the apostrophe as opening quoted text, although Log4j2 treats it as a literal regex character. The message and correlation fields end up on separate lines, with the fields running into the next event. Restrict this quote handling to Logback.
| } else if (c == '\'' || c == '"') { | |
| } else if (logback && (c == '\'' || c == '"')) { |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| } | ||
|
|
||
| private static boolean isComposite(String name) { | ||
| return "style".equals(name) |
There was a problem hiding this comment.
The composite list omits Log4j2's built-in color wrappers and maxLength alias, so existing MDC converters inside them are missed. For example, %cyan{%X{dd.trace_id}} %m%n gains a duplicate trace ID, while %blue{%X} %m%n gains all three fields despite already printing the entire MDC. Recognize these wrappers to preserve the promise to add only missing fields.
| return "style".equals(name) | |
| return "black".equals(name) | |
| || "blue".equals(name) | |
| || "cyan".equals(name) | |
| || "green".equals(name) | |
| || "magenta".equals(name) | |
| || "red".equals(name) | |
| || "white".equals(name) | |
| || "yellow".equals(name) | |
| || "maxLength".equals(name) | |
| || "style".equals(name) |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
|
|
||
| @Override | ||
| public String instrumentedType() { | ||
| return "org.apache.logging.log4j.core.impl.Log4jLogEvent"; |
There was a problem hiding this comment.
On Log4j2 versions before 2.7, AsyncLogger delivers RingBufferLogEvent instances directly to appenders, bypassing the Log4jLogEvent constructors instrumented here. The legacy ThreadContext updater supplies only trace/span IDs, and the newer context-data injector is unavailable. Consequently, enabling pattern injection leaves the added service field empty despite a configured service name. Service enrichment must also cover asynchronous event population.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74d4ab92a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| @Override | ||
| public String instrumentedType() { | ||
| return "org.apache.logging.log4j.core.impl.Log4jLogEvent"; |
There was a problem hiding this comment.
When Log4j 2.0–2.6 is configured with AsyncLogger, events are populated as RingBufferLogEvent rather than constructed as Log4jLogEvent, so this advice is never invoked (and no other instrumentation in the Log4j modules targets that event type). The pre-2.7 scope listener only places trace/span IDs in ThreadContext, so the newly inserted %X{dd.service} renders empty even with logs injection enabled; cover the async event path as well.
Useful? React with 👍 / 👎.
| private static boolean isComposite(String name) { | ||
| return "style".equals(name) | ||
| || "highlight".equals(name) |
There was a problem hiding this comment.
When a supported Log4j pattern wraps an existing correlation converter in %upperCase{...}, %lowerCase{...}, or the maxLength alias, fields() only descends into names accepted by this incomplete allowlist. It therefore fails to mark the nested key and injects a second converter (for %upperCase{%X{dd.trace_id}}, the ID is printed twice), so include all Log4j composite names and aliases.
Useful? React with 👍 / 👎.
What Does This Do
Add
DD_LOGS_PATTERN_INJECT=true(-Ddd.logs.pattern.inject=true) to make service, trace ID, and span ID visible in existing Logback classic and Log4j2 plain-text logs. The option is disabled by default. It adds only missing MDC converters after the first complete field, preserving timestamp options and the rest of the application's pattern.Motivation
MDC injection alone does not display correlation fields when a text layout does not reference their keys. Applications currently have to update each logging configuration or replace the complete pattern, which can disrupt the timestamp prefix expected by log collectors.
Solves #13276.
Additional Notes
Pattern parsing runs during layout initialization. Logback uses its native parser to recognize existing converters; Log4j2 handles both legacy single-key MDC options and newer key lists. Layout restarts do not duplicate fields. Existing application MDC values are preserved, and
DD_LOGS_INJECTIONcontinues to control correlation values.On Log4j2 versions before 2.7, adding service requires enriching the event's context map; a nonempty application map is copied to preserve its contents. This path is enabled only with the new option. Newer versions reuse the existing context-data injector.
JSON layouts, Logback access layouts, and custom Log4j2 PatternSelector branches are outside this change. Usage and configuration details are documented in
docs/log-pattern-injection.md.Validation on JDK 25:
Contributor Checklist
Requested labels:
type: feature,comp: logs,tag: ai generated.