Uploaded image for project: 'Log4j 2'
  1. Log4j 2
  2. LOG4J2-741

Reinstate the package attribute for discovering custom plugins

Details

    • Improvement
    • Status: Closed
    • Blocker
    • Resolution: Fixed
    • 2.0-rc2, 2.0
    • 2.0.1
    • Core
    • None

    Description

      Several people reported problems with their custom plugins no longer being recognized by log4j2. See LOG4J2-673 and this StackOverflow question.

      Plugins created before the annotation processor was added to log4j2 (all plugins created with 2.0-rc1 and earlier) may not have a META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat file.

      Previously plugins without this metadata file could still be found if the user specified their custom plugin package(s) in the packages attribute of the <Configuration> element in their log4j2.xml configuration file.

      However, since 2.0-rc2, the packages configuration attribute was disabled; users may still specify a value, but log4j2 will no longer use this value to try to load custom plugins. This causes problems for custom plugins built before the annotation processor was added to log4j2, as well as custom plugins that are built in an environment where the annotation processor does not work (for example, most IDEs require some setting changes to enable annotation processing).

      This Jira ticket is to reactivate the packages configuration attribute.

      Attachments

        1. LOG4J2-741-patch.txt
          10 kB
          Remko Popma

        Issue Links

          Activity

            mattsicker Matt Sicker added a comment -

            One way to go about doing this I think would be invoking the annotation processor programmatically for packages specified by this. It would still be done at runtime instead of through an executable tool (since you can use the annotation processor through javac as it is for the executable tool). I have an idea on how to implement this and will experiment on it in the upcoming days.

            mattsicker Matt Sicker added a comment - One way to go about doing this I think would be invoking the annotation processor programmatically for packages specified by this. It would still be done at runtime instead of through an executable tool (since you can use the annotation processor through javac as it is for the executable tool). I have an idea on how to implement this and will experiment on it in the upcoming days.
            rpopma Remko Popma added a comment -

            Is that a good idea? Javac would require another runtime dependency: custom plugins would not be recognized unless tools.jar is in the classpath.

            I would prefer to just revert the previous code. I started to do some work on this, see attached file LOG4J2-741-patch.txt for what I have in mind. This is still a work in progress - the build works but I haven't tested it.

            rpopma Remko Popma added a comment - Is that a good idea? Javac would require another runtime dependency: custom plugins would not be recognized unless tools.jar is in the classpath. I would prefer to just revert the previous code. I started to do some work on this, see attached file LOG4J2-741 -patch.txt for what I have in mind. This is still a work in progress - the build works but I haven't tested it.
            jedws Jed Wesley-Smith added a comment - - edited

            I strongly consider the removal of this feature in a late RC a serious bug, and a violation of the RC process. I wasted a fair amount of time trying to diagnose why things that worked previously were no longer working, with no information anywhere that the documented configuration element "packages" actually doesn't do anything.

            Eventually I discovered this had been replaced as the javac annotation processor can be used to create an obscure file in the jar. In our case we do not use javac, we are writing our plugins in Scala. We (and others in other JVM languages) need a way to load our plugins that isn't tied to a specific compiler.

            By the way the documentation still refers to the PluginManager that no longer works.

            jedws Jed Wesley-Smith added a comment - - edited I strongly consider the removal of this feature in a late RC a serious bug, and a violation of the RC process. I wasted a fair amount of time trying to diagnose why things that worked previously were no longer working, with no information anywhere that the documented configuration element "packages" actually doesn't do anything. Eventually I discovered this had been replaced as the javac annotation processor can be used to create an obscure file in the jar. In our case we do not use javac, we are writing our plugins in Scala. We (and others in other JVM languages) need a way to load our plugins that isn't tied to a specific compiler. By the way the documentation still refers to the PluginManager that no longer works.
            mattsicker Matt Sicker added a comment -

            The documentation is updated in the trunk and will be fixed in 2.0.1. Can you not use javac and -proc:only on the class files that scalac creates? I'm not too familiar with the Scala tools, and googling hasn't really found anything useful in regard to annotation processing.

            mattsicker Matt Sicker added a comment - The documentation is updated in the trunk and will be fixed in 2.0.1. Can you not use javac and -proc:only on the class files that scalac creates? I'm not too familiar with the Scala tools, and googling hasn't really found anything useful in regard to annotation processing.
            mattsicker Matt Sicker added a comment -

            I think using something like Reflections would be useful for porting the annotation processor. I'm surprised at the lack of support outside Java!

            mattsicker Matt Sicker added a comment - I think using something like Reflections would be useful for porting the annotation processor. I'm surprised at the lack of support outside Java!

            I don't know how you'd ask javac to process .class files

            You're surprised that javac plugins don't work outside of javac?

            jedws Jed Wesley-Smith added a comment - I don't know how you'd ask javac to process .class files You're surprised that javac plugins don't work outside of javac?
            rpopma Remko Popma added a comment -

            I am not in favor of adding any runtime dependency in order to make custom plugins work, whether it is tools.jar, Reflections or other.

            Jed, I agree that support for the packages attribute should not have been removed without at least a mention in the release notes and the web site when the change was made. I am not sure how much awareness there was in the team regarding this change. (Speaking for myself, I completely missed this...)

            Anyway, yours is not the only feedback on this issue. Several people have complained that custom plugins that used to work with older versions of log4j2 no longer work. I apologize for the inconvenience and we will try to rectify this in the next maintenance release.

            rpopma Remko Popma added a comment - I am not in favor of adding any runtime dependency in order to make custom plugins work, whether it is tools.jar, Reflections or other. Jed, I agree that support for the packages attribute should not have been removed without at least a mention in the release notes and the web site when the change was made. I am not sure how much awareness there was in the team regarding this change. (Speaking for myself, I completely missed this...) Anyway, yours is not the only feedback on this issue. Several people have complained that custom plugins that used to work with older versions of log4j2 no longer work. I apologize for the inconvenience and we will try to rectify this in the next maintenance release.
            rpopma Remko Popma added a comment -

            Matt, you assigned yourself to this issue when you were still thinking of an annotation processor-based solution. I think reverting the old code (if that is possible) is preferable as it has a track record and it would avoid adding runtime dependencies.

            I can work on a solution along those lines tomorrow (Saturday Japan time). In that case, I would really like one or more people to help with a review though. On the other hand, you already picked up this item and I don't want to be stepping on your toes. How would you like to proceed with this issue?

            rpopma Remko Popma added a comment - Matt, you assigned yourself to this issue when you were still thinking of an annotation processor-based solution. I think reverting the old code (if that is possible) is preferable as it has a track record and it would avoid adding runtime dependencies. I can work on a solution along those lines tomorrow (Saturday Japan time). In that case, I would really like one or more people to help with a review though. On the other hand, you already picked up this item and I don't want to be stepping on your toes. How would you like to proceed with this issue?
            mattsicker Matt Sicker added a comment -

            I agree on reverting. I'll help review. We can work on cleaning up duplicate code into proper abstractions later on. There was some similar duplication I wanted to handle anyway, so adding in runtime plugin scanning is not a big deal. The plugin processor and runtime version will just need to be refactored later.

            I tested out your patch last night and it seemed to work, but I don't have any Scala plugins to test it with.

            mattsicker Matt Sicker added a comment - I agree on reverting. I'll help review. We can work on cleaning up duplicate code into proper abstractions later on. There was some similar duplication I wanted to handle anyway, so adding in runtime plugin scanning is not a big deal. The plugin processor and runtime version will just need to be refactored later. I tested out your patch last night and it seemed to work, but I don't have any Scala plugins to test it with.
            rpopma Remko Popma added a comment -

            Understood, thanks! I'm starting work on this now.

            rpopma Remko Popma added a comment - Understood, thanks! I'm starting work on this now.
            rpopma Remko Popma added a comment -

            Fixed in revision 1613615.

            Added a JUnit test with a custom plugin loaded via a configuration file with the packages attribute, but reviews would be great.

            For example, I reverted back the old code in PluginManager.addPackages() that looks like this:

            public static void addPackage(final String p) {
                if (PACKAGES.addIfAbsent(p)) {
                    // set of available plugins could have changed, reset plugin cache for newly-retrieved managers
                    REGISTRY.clear(); // TODO confirm if this is correct
                }
            }
            

            Should the REGISTRY really be cleared here? This removes all plugins found in Log4j2Plugins.dat files earlier. I am not sure if this is a problem because as far as I can tell the addPackages method is called first, before the loadPlugins method.

            rpopma Remko Popma added a comment - Fixed in revision 1613615. Added a JUnit test with a custom plugin loaded via a configuration file with the packages attribute, but reviews would be great. For example, I reverted back the old code in PluginManager.addPackages() that looks like this: public static void addPackage( final String p) { if (PACKAGES.addIfAbsent(p)) { // set of available plugins could have changed, reset plugin cache for newly-retrieved managers REGISTRY.clear(); // TODO confirm if this is correct } } Should the REGISTRY really be cleared here? This removes all plugins found in Log4j2Plugins.dat files earlier. I am not sure if this is a problem because as far as I can tell the addPackages method is called first, before the loadPlugins method.
            mattsicker Matt Sicker added a comment - - edited

            All looks good!

            Edit: in regards to the packages clear, that doesn't really look right. It could lose plugins that are still available but not from the same classpath.

            mattsicker Matt Sicker added a comment - - edited All looks good! Edit: in regards to the packages clear, that doesn't really look right. It could lose plugins that are still available but not from the same classpath.
            rpopma Remko Popma added a comment -

            About the registry.clear in addPackage, I see your point. Still, I hesitate to remove it until I understand why the clear was necessary in the first place. Do you know what the clear() is for?

            One more thing, running PluginManagerPackagesTest (as a JUnit test from Eclipse), creates a file /log4j-core/META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat.

            Running the tests from maven creates this file: /log4j-core/Log4j2Plugins.dat.

            I would like these files to be created under /log4j-core/target, but so far I haven't figured out where they get created.

            rpopma Remko Popma added a comment - About the registry.clear in addPackage , I see your point. Still, I hesitate to remove it until I understand why the clear was necessary in the first place. Do you know what the clear() is for? One more thing, running PluginManagerPackagesTest (as a JUnit test from Eclipse), creates a file /log4j-core/META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat. Running the tests from maven creates this file: /log4j-core/Log4j2Plugins.dat. I would like these files to be created under /log4j-core/target, but so far I haven't figured out where they get created.
            mattsicker Matt Sicker added a comment -

            I can tell you that the registry clear will affect other configs (e.g., different WARs, different bundles). I think the idea was that if another config showed up with the same packages attribute (e.g., a reconfigure), then the registry might be stale. I think the registry will need to be enhanced with proper lifecycle methods all around. This would certainly be a 2.1 thing at best.

            mattsicker Matt Sicker added a comment - I can tell you that the registry clear will affect other configs (e.g., different WARs, different bundles). I think the idea was that if another config showed up with the same packages attribute (e.g., a reconfigure), then the registry might be stale. I think the registry will need to be enhanced with proper lifecycle methods all around. This would certainly be a 2.1 thing at best.
            rpopma Remko Popma added a comment -

            I've thought about this a little but I'm still not convinced that there is actually any harm in clearing the registry in addPackage. The registry is only used for configuration. Every time a configuration is started, PluginManager.collectPlugins() is called, which in turn calls loadPlugins. So even if the registry was cleared it will be initialized again if necessary, no?

            rpopma Remko Popma added a comment - I've thought about this a little but I'm still not convinced that there is actually any harm in clearing the registry in addPackage. The registry is only used for configuration. Every time a configuration is started, PluginManager.collectPlugins() is called, which in turn calls loadPlugins . So even if the registry was cleared it will be initialized again if necessary, no?
            mattsicker Matt Sicker added a comment -

            The way that plugins are registered through OSGi is done at install time for each bundle. It's not repeated unless you refresh the bundle for instance, and that requires restarting everything as it is.

            mattsicker Matt Sicker added a comment - The way that plugins are registered through OSGi is done at install time for each bundle. It's not repeated unless you refresh the bundle for instance, and that requires restarting everything as it is.
            ianbarfield Ian Barfield added a comment - - edited

            This change has made 2.0.1+ unusable for me. It takes about 3 seconds to perform PluginManager.collectPlugins() and it appears to run 4 times before log4j2 is done initializing. That is pretty not okay. Please consider making the plugin discovery method configurable and/or curtailing redundant calls.

            ianbarfield Ian Barfield added a comment - - edited This change has made 2.0.1+ unusable for me. It takes about 3 seconds to perform PluginManager.collectPlugins() and it appears to run 4 times before log4j2 is done initializing. That is pretty not okay. Please consider making the plugin discovery method configurable and/or curtailing redundant calls.

            @ianbarfield OOI, do you specify a "packages" attribute in the log4j config? I am not a contributor, nor an expert on this, but I wonder if either it is specified to something small (or non-existent) then it shouldn't do much scanning at all.

            I also wonder if it should scan at all if the attribute is not present, but that's just thinking aloud, I don't actually know how it works atm.

            jedws Jed Wesley-Smith added a comment - @ianbarfield OOI, do you specify a "packages" attribute in the log4j config? I am not a contributor, nor an expert on this, but I wonder if either it is specified to something small (or non-existent) then it shouldn't do much scanning at all. I also wonder if it should scan at all if the attribute is not present, but that's just thinking aloud, I don't actually know how it works atm.
            rpopma Remko Popma added a comment -

            Ian, we can definitely look into this. Would you mind creating a new issue for the startup performance problems you are seeing? (Perhaps with a link to this issue.)

            rpopma Remko Popma added a comment - Ian, we can definitely look into this. Would you mind creating a new issue for the startup performance problems you are seeing? (Perhaps with a link to this issue.)
            ianbarfield Ian Barfield added a comment - created here: https://issues.apache.org/jira/browse/LOG4J2-798
            mattsicker Matt Sicker added a comment -

            Closing this as it was done a while back.

            mattsicker Matt Sicker added a comment - Closing this as it was done a while back.
            Frank.Conover Frank Conover added a comment -

            I am experiencing this issue with 2.6.3 and I tried 2.3. I have a custom plugin in a jar file. Works fine from eclipse, or a non-jar java app but not as a jar project. I have packages="... attribute filled out in my log4j2.xml. I added it to get the custom appender found while running from eclipse. As a non-maven program using jars which are on the classpath the appender is not found. Is there another way I should be doing this? Is packages still fixed / or still an available feature?
            Note: If I extract my jars and run, the appender is found.

            The stack overflow link in the description sounds like my issue. Do I need to compile with a certain flag turned on? Generate my jar a certain way?

            Frank.Conover Frank Conover added a comment - I am experiencing this issue with 2.6.3 and I tried 2.3. I have a custom plugin in a jar file. Works fine from eclipse, or a non-jar java app but not as a jar project. I have packages="... attribute filled out in my log4j2.xml. I added it to get the custom appender found while running from eclipse. As a non-maven program using jars which are on the classpath the appender is not found. Is there another way I should be doing this? Is packages still fixed / or still an available feature? Note: If I extract my jars and run, the appender is found. The stack overflow link in the description sounds like my issue. Do I need to compile with a certain flag turned on? Generate my jar a certain way?
            rpopma Remko Popma added a comment -

            We may need to do some digging with extra debug info printed to the status logger to find out what's going on. Can you open a new ticket with your description and the output of trace-level status logging for the case where the custom plugin is not found?

            rpopma Remko Popma added a comment - We may need to do some digging with extra debug info printed to the status logger to find out what's going on. Can you open a new ticket with your description and the output of trace-level status logging for the case where the custom plugin is not found?

            People

              rpopma Remko Popma
              rpopma Remko Popma
              Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: