Log4j 2
  1. Log4j 2
  2. LOG4J2-741

Reinstate the package attribute for discovering custom plugins

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0-rc2, 2.0
    • Fix Version/s: 2.0.1
    • Component/s: Core
    • Labels:
      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.

        Issue Links

          Activity

          Hide
          Matt Sicker added a comment -

          Closing this as it was done a while back.

          Show
          Matt Sicker added a comment - Closing this as it was done a while back.
          Hide
          Ian Barfield added a comment -
          Show
          Ian Barfield added a comment - created here: https://issues.apache.org/jira/browse/LOG4J2-798
          Hide
          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.)

          Show
          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.)
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          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?

          Show
          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?
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          Remko Popma added a comment -

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

          Show
          Remko Popma added a comment - Understood, thanks! I'm starting work on this now.
          Hide
          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.

          Show
          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.
          Hide
          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?

          Show
          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?
          Hide
          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.

          Show
          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.
          Hide
          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?

          Show
          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?
          Hide
          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!

          Show
          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!
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.

            People

            • Assignee:
              Remko Popma
              Reporter:
              Remko Popma
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development