apache / logging-log4j2

Apache Log4j 2 is a versatile, feature-rich, efficient logging API and backend for Java.
https://logging.apache.org/log4j/2.x/
Apache License 2.0
3.4k stars 1.62k forks source link

ContextDataInjector ignores custom ContextDataProvider #2331

Closed lukasalexanderweber closed 2 months ago

lukasalexanderweber commented 8 months ago

Description

Per documentation I should be able to create a custom ContextDataProvider. However, when following the documented steps, the context is not resolved. This is also reported here.

I created a minimal reproducible example where I followed the steps described in the docs:

  1. Custom implementations should implement the org.apache.logging.log4j.core.util.ContextDataProvider interface (MyContextDataProvider)
  2. and declare it as a service by defining the implementation class in a file named META-INF/services/org.apache.logging.log4j.core.util.ContextDataProvider (org.apache.logging.log4j.core.util.ContextDataProvider)

I created a unit test with a log4j2-test.properties setup and expect for the path src/test/logs/${ctx:tenant} that the tenant is resolved by MyContextDataProvider. However, an error is printed that the file with the unresolved tenant cannot be created. It works as expected when the tenant is set via ThreadContext.put("tenant", "tenant1");

Configuration

Version: 2.23.0

Operating system: Windows 10

JDK: Corretto-17.0.8.8.1

Logs

2024-03-01T08:40:19.499742800Z main ERROR Unable to create file src/test/logs/${ctx:tenant}/logs.log java.io.IOException: Die Syntax für den Dateinamen, Verzeichnisnamen oder die Datenträgerbezeichnung ist falsch
    at java.base/java.io.WinNTFileSystem.canonicalize0(Native Method)
    at java.base/java.io.WinNTFileSystem.canonicalize(WinNTFileSystem.java:462)
    at java.base/java.io.File.getCanonicalPath(File.java:626)
    at java.base/java.io.File.getCanonicalFile(File.java:651)
    at org.apache.logging.log4j.core.util.FileUtils.makeParentDirs(FileUtils.java:141)
    at org.apache.logging.log4j.core.appender.rolling.RollingFileManager$RollingFileManagerFactory.createManager(RollingFileManager.java:863)

Reproduction

see ContextDataProviderMRE

ppkarwasz commented 8 months ago

Thank you for the report.

Technically ContextDataInjector#injectContextData works correctly, so you'll see your "context" in your logs, but the [ContextDataInjector#rawContextData](https://logging.apache.org/log4j/2.x/javadoc/log4j-core/org/apache/logging/log4j/core/ContextDataInjector.html#rawContextData()) method doesn't, so you don't see it in the context lookup. Since Windows does not accept : in path names, you see only half of the picture.

We'll look into that, although at first sight it might not be an easy improvement considering the performance restrictions.

In the meantime, you could write a StrLookup with the same data.

rgoers commented 7 months ago

I am going to go one further. This is NOT a bug. The purpose of ContextDataProvider is to inject data into LogEvents. That is documented clearly in the Extending Log4j section of the manual. Now because the Context Lookup inspects data in the LogEvent when it is passed on it will work as you expect in that circumstance. But where no LogEvent exists it cannot possibly know where your extra data is located.

Therefore I am closing this.

rgoers commented 7 months ago

Well, I am going to reopen this as PR https://github.com/apache/logging-log4j2/pull/2438 actually fixes this.

lukasalexanderweber commented 7 months ago

great! Till now we used ContextDataInjector to create the files and map the logs but we saw that it's recommended to switch to ContextDataProvider and then we were suprised that this functionality no longer works.

NOTE: It is no longer recommended that custom implementations of this interface be provided as it is difficult to do. Instead, provide a custom ContextDataProvider.

lukasalexanderweber commented 6 months ago

@rgoers the PR you mentioned is merged, is this issue resolved?

rgoers commented 6 months ago

We have not released 2.24.0 yet. You can try with the latest SNAPSHOT

ppkarwasz commented 3 months ago

We postponed the merge of #2438 until 2.25.0. However I have backported the part that fixes this issue in #2846. It is planned to be released in 2.24.0.