search

qos-ch / logback

The reliable, generic, fast and flexible logging framework for Java.
http://logback.qos.ch
Other
2.97k stars 1.28k forks source link

PatternLayoutBase custom converter classloading trouble in OSGi #803

Open enapps-enorman opened 4 months ago

enapps-enorman commented 4 months ago

The values in the PatternLayoutBase defaultConverterMap and instanceConverterMap are class names. The ch.qos.logback.core.pattern.parser.Compiler class attempts to load the class for those class names with the assumption that those classes are visible to the classloader that loaded the logback-core classes.

Unfortunately, in an OSGi environment, my custom converter classes from a custom OSGi bundle are not visible to the classloader of the logback-core bundle. This scenario results in a "Failed to instantiate converter class" error and it doesn't work as expected.

My proposal is to allow the the values of the defaultConverterMap and instanceConverterMap to alternately be supplied as a Supplier function instead of a classname. The Supplier would not have the same class visibility problem.

Using a Supplier seems to be a technique that I see in other places in the logback codebase, so it seems like it should make sense to allow that for this scenario as well.

I will supply a PR with the proposed changes for your consideration

enapps-enorman commented 6 days ago

Hi @ceki

Checking back in again since we haven't gotten any feedback on the PR since it was proposed 4 months ago.

We would appreciate a review of this proposal if you (or someone else) has the time. Knowing if this proposal will be accepted or not will help us decide how to move forward.

Thanks

ceki commented 5 days ago

Hi @enapps-enorman,

Thank you for this PR. I imagine that the suppliers would need to be created by the OSGi platform or the application itself. How/when will these suppliers be created?

How do you intend to inject the suppliers into logback? You would need hooks in logback itself but these seem to be missing in the PR.

enapps-enorman commented 5 days ago

Hi @ceki

Thanks for your response. This proposal was to resolve a problem discovered during work to migrate existing code from older slf4j(1.x)+logback(1.2.x) versions to the latest releases and the refactoring needed to deal with everything that had changed. For your reference, that pending PR remains at: https://github.com/apache/sling-org-apache-sling-commons-log/pull/18

Thank you for this PR. I imagine that the suppliers would need to be created by the OSGi platform or the application itself. How/when will these suppliers be created?

Yes, our use case does involve some significant customization of the logback init logic for modularization. This was to support supplying the log config more dynamically based on which OSGi configuration+bundles are active in the application runtime.

For your reference, those logback integration customization details are described briefly in the documentation at: https://sling.apache.org/documentation/development/logging.html#logback-integration

How do you intend to inject the suppliers into logback? You would need hooks in logback itself but these seem to be missing in the PR.

The custom logging init code basically has a custom factory for various PatternLayout objects that programmatically populates the instance converter supplier map with the supplier lambda expressions. This output "masking" code was written by someone else and I believe the purpose was to mask certain characters in the output for https://issues.apache.org/jira/browse/SLING-11393

For your reference, the following is the method (from a pending PR) that customizes the PatternLayout with the suppliers: https://github.com/enapps-enorman/sling-org-apache-sling-commons-log/blob/issue/SLING-11906/src/main/java/org/apache/sling/commons/log/logback/internal/MaskingMessageUtil.java#L53

Does that make sense?