Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Logging framework uses MDC adapter provided by SubstituteServiceProvider even after initialization of CustomServiceProvider #450

Closed
OmShinde1513 opened this issue Feb 1, 2025 · 9 comments
Closed

Logging framework uses MDC adapter provided by SubstituteServiceProvider even after initialization of CustomServiceProvider #450

OmShinde1513 opened this issue Feb 1, 2025 · 9 comments
Assignees
@ceki
Labels
BUG DONE for fixed issues
Milestone
2.0.17

Comments

@OmShinde1513
Copy link

Hi,
I'm Using logback 1.5.16 logging framework and slf4j 2.0.16,
when LogbackServiceProvider initialization is ongoing, slf4j provides SubstituteServiceProvider,
In static block of MDC class, mdcAdapter is initialized with BasicMDCAdapter provided by SubstituteServiceProvider, and this is not replaced with MDCAdapter provided by LogbackServiceProvider after service providers initialization completed.
Hence MDC key values are placed in this BasicMDCAdapter, But logback appenders get mdcPropertyMap from its own MDCAdapter so this map gets empty as all property values placed in map of BasicMDCAdapter.
ex:
// After custome service initialization completed.
MDC.put(key,value) -> puts this in BasicMDCAdapter provided by SubstituteServiceProvider.
loggerContext.getMDCAdapter() -> this adapter have empty PropertyMap, which is used by logback to set mdc properties to LoggingEvent.

After initialization of CustomServiceProvider MDCAdapter should be replaced with its MDCAdapter.
@ceki ceki self-assigned this Feb 2, 2025
@ceki ceki added the BUG label Feb 2, 2025
@OmShinde1513
Copy link
Author

OmShinde1513 commented Feb 3, 2025 •
edited
Loading

Hi @ceki,
We are currently facing security issues with the older version of Logback, and as a result, we are in the process of upgrading both Logback and SLF4J, this security issues are critical for us.
Could you please provide an update on when this issue will be resolved?

@viphadke
Copy link

viphadke commented Feb 5, 2025

@ceki Thanks for looking into the issue.
The issue is a blocker for our release. Do you have any estimate on when the problem can be fixed and released?

@ceki ceki added this to the 2.0.17 milestone Feb 6, 2025
ceki added a commit that referenced this issue Feb 7, 2025
@ceki
In response to issue 450, moved the binding of the MDCAdapter in the
9349d64 
MDC class from the static initializer to the LoggerFactory.bind method.

In case MDC methods are called before LoggerFactory, the various methods
in MDC have been modified to go through the getMDCAdapter method which
checks for this rare case.

#450

Signed-off-by: Ceki Gulcu <[email protected]>
@ceki
Copy link
Member

ceki commented Feb 7, 2025

@OmShinde1513 This issue is solved in commit 9349d64

@ceki ceki added DONE for fixed issues and removed IN_PROGRESS labels Feb 7, 2025
@skanikdale
Copy link

@ceki Thanks a lot for the quick turnaround. When will the next release be available which will contain this fix?

@OmShinde1513
Copy link
Author

OmShinde1513 commented Feb 9, 2025 •
edited
Loading

Hi @ceki Thanks for looking into issue,
Since in current version of logback the LogbackServiceProvider not initializes MDCAdapter field in its constructor or on field declaration, due to this LoggerFactory#earlyBindMDCAdapter not able to initialize the MDC_ADAPTER. As a result, the MDC_ADAPTER provided by the SubstituteServiceProvider is used throughout the entire application and MDC's are not getting in logs.
Is there will be new release of logback for this ?

@viphadke
Copy link

viphadke commented Feb 11, 2025 •
edited
Loading

@ceki The fix is not working for use.
Let me try to clarify @OmShinde1513 comment.

I think this might be very specific to our usage of logback and sl4j.
We have a logback lifecycle listener where we use MDC. This retriggers another initialization of the service provider.
I am attaching the source code here.

App.txt
CustomLoggerContextListener.txt

logback.txt

Thanks for looking into this.

@ceki
Copy link
Member

ceki commented Feb 11, 2025

@viphadke Thank you for the helpful clarification.

@viphadke
Copy link

@ceki Do you have any ETA on a possible solution ?

ceki added a commit that referenced this issue Feb 25, 2025
@ceki
In response to issue 450, moved the binding of the MDCAdapter in the
9bd0374 
MDC class from the static initializer to the LoggerFactory.bind method.

In case MDC methods are called before LoggerFactory, the various methods
in MDC have been modified to go through the getMDCAdapter method which
checks for this rare case.

#450

Signed-off-by: Ceki Gulcu <[email protected]>
@ceki
Copy link
Member

ceki commented Feb 25, 2025 •
edited
Loading

Logback version 1.5.17 has been released a short time ago. See also https://logback.qos.ch/news.html#1.5.17

@ceki ceki closed this as completed Feb 25, 2025
@LuciferYang LuciferYang mentioned this issue Mar 3, 2025
Closed
dongjoon-hyun pushed a commit to apache/spark that referenced this issue Mar 4, 2025
@LuciferYang @dongjoon-hyun
[SPARK-51367][BUILD] Upgrade slf4j to 2.0.17
8f0ac3b 
### What changes were proposed in this pull request?
This pr aims to upgrade slf4j from 2.0.16 to 2.0.17.

### Why are the changes needed?
The new version brings some bug fixes, like:

- As reported in qos-ch/slf4j#450, in some rare cases where MDC could be initialized before LoggerFactory. Thus, MDC would be stuck using the wrong MDCAdapter instance. To fix this issue LoggerFactory and MDC have been modified. Implementations of SLF4JServiceProvider are encouraged to initialize their mdcAdapter and markerFactory fields as early as possible, preferably at construction time. Note that these changes are transparent to existing logging backends which will continue to work as is.

- Fixed incorrect interpretation of Level.OFF and Level.ALL in SLF4JPlatformLogger by mapping Level.OFF as Level.ERROR and Level.ALL as Level.TRACE. This issue was reported in qos-ch/slf4j#430 by Peter Halicky.

The full release notes as follows:
- https://github.com/qos-ch/slf4j/releases/tag/v_2.0.17

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #50115 from LuciferYang/slf4j-2.0.17.

Lead-authored-by: yangjie01 <[email protected]>
Co-authored-by: YangJie <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@bbodnar bbodnar mentioned this issue Mar 6, 2025
Open
Pajaraja pushed a commit to Pajaraja/spark that referenced this issue Mar 6, 2025
@LuciferYang
[SPARK-51367][BUILD] Upgrade slf4j to 2.0.17
322040a 
### What changes were proposed in this pull request?
This pr aims to upgrade slf4j from 2.0.16 to 2.0.17.

### Why are the changes needed?
The new version brings some bug fixes, like:

- As reported in qos-ch/slf4j#450, in some rare cases where MDC could be initialized before LoggerFactory. Thus, MDC would be stuck using the wrong MDCAdapter instance. To fix this issue LoggerFactory and MDC have been modified. Implementations of SLF4JServiceProvider are encouraged to initialize their mdcAdapter and markerFactory fields as early as possible, preferably at construction time. Note that these changes are transparent to existing logging backends which will continue to work as is.

- Fixed incorrect interpretation of Level.OFF and Level.ALL in SLF4JPlatformLogger by mapping Level.OFF as Level.ERROR and Level.ALL as Level.TRACE. This issue was reported in qos-ch/slf4j#430 by Peter Halicky.

The full release notes as follows:
- https://github.com/qos-ch/slf4j/releases/tag/v_2.0.17

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#50115 from LuciferYang/slf4j-2.0.17.

Lead-authored-by: yangjie01 <[email protected]>
Co-authored-by: YangJie <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
anoopj pushed a commit to anoopj/spark that referenced this issue Mar 15, 2025
@LuciferYang @anoopj
[SPARK-51367][BUILD] Upgrade slf4j to 2.0.17
d871e85 
### What changes were proposed in this pull request?
This pr aims to upgrade slf4j from 2.0.16 to 2.0.17.

### Why are the changes needed?
The new version brings some bug fixes, like:

- As reported in qos-ch/slf4j#450, in some rare cases where MDC could be initialized before LoggerFactory. Thus, MDC would be stuck using the wrong MDCAdapter instance. To fix this issue LoggerFactory and MDC have been modified. Implementations of SLF4JServiceProvider are encouraged to initialize their mdcAdapter and markerFactory fields as early as possible, preferably at construction time. Note that these changes are transparent to existing logging backends which will continue to work as is.

- Fixed incorrect interpretation of Level.OFF and Level.ALL in SLF4JPlatformLogger by mapping Level.OFF as Level.ERROR and Level.ALL as Level.TRACE. This issue was reported in qos-ch/slf4j#430 by Peter Halicky.

The full release notes as follows:
- https://github.com/qos-ch/slf4j/releases/tag/v_2.0.17

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#50115 from LuciferYang/slf4j-2.0.17.

Lead-authored-by: yangjie01 <[email protected]>
Co-authored-by: YangJie <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ceki ceki

Labels
BUG DONE for fixed issues
Projects
None yet
Milestone
2.0.17
Development

No branches or pull requests
4 participants
@ceki @skanikdale @OmShinde1513 @viphadke