That's a fantastic overview, Ralph! I'd add a hint to the PluginManager control in that even without a full DI system update, if you can identify an appropriate anchor point to load and store the PluginManager instance, then things will fit fairly naturally into the existing system. Many of the anchors in current use are things like the Configuration instance (which is itself injectable via @PluginConfiguration), a ThreadLocal value somewhere, some external API that tracks objects like a ServletContext, etc.
On Sat, 30 Jan 2021 at 14:51, Ralph Goers <ralph.go...@dslextreme.com> wrote: > > You have to remember that Log4j 2 started from looking at Log4j 1 and > Logback. One of the things I hated was that both of the predecessors > required class names to be specified in the logging configuration. The > original incarnation of the plugin system was built solely to remove the need > for class names to be specified like that. > > See below. > > > > On Jan 29, 2021, at 12:15 PM, Volkan Yazıcı <volkan.yaz...@gmail.com> wrote: > > > > a) I understand the policy that core shouldn't have any dependencies. > > b) Maybe `ServiceLoader`s? > > Release 2.x loaded Log4jPlugins.dat as system resources. Master has been > modified to load plugins via ServiceLoader. This is required to work with the > Java module system. > > > c) I see your point and agree with that. But replacing `new > > PluginManager(T.class).getPlugins()` with injection via `@PluginElement > > Set<T> plugins` shouldn't be complicating the current system that much, I > > guess. > > > > Classpath scanning is indeed a pain point regarding the start-up > > performance and `PluginManager#getPlugins` is doing exactly that. > > Classpath scanning should only happen if packages are specified to scan. The > first versions of Log4j 2 didn’t have the annotation processor so classpath > scanning was required. Once the annotation processor was introduced the need > for classpath scanning should no longer exist. In Master the Log4jPlugins > class that is generated by the annotation processor will contain all the > defined plugins in a jar (or module). We use ServiceLoader to load all > instances of that class to locate all the plugins. > > The “normal” way components define plugins is by specifying them with their > own category. For example, all converters for the Pattern Converter are > defined with Category = “Converter”, Lookups are defined with Category = > “Lookup”, Flume secret key providers are defined with Category = > “KeyProvider”, etc. Al in all we probably have 8 or 10 kinds of plugins. This > is why the PluginManager’s constructor is called with the category. It > retrieves the plugins of that category from the plugin repository and the > requestor then only has access to that set of plugins. > > So in your case you could create an EventResolver category and then be able > to configure them on JsonTemplateLayout from the template configuration. > > Injection has only really been a problem in that lots of plugins typically > want access to things like the Configuration (mostly to get at the > StrSubstitutor), and occasionally the LoggerContext. We took shortcuts to > provide those since there wasn’t a great need to make it more general. > > In other words, when the plugin system was designed it was never intended to > be a dependency injection system. It was simply a way to allow users to > create custom components of a certain type that they could easily create and > easily allow to be used. It is extremely simple for someone to create a new > Appender, Filter, or Lookup and have it be used in their logging > configuration. All these extra usages have grown out of that as we have > discovered places where added flexibility would be nice. > > Remember, if you call PluginManager that means you are controlling the > “loading” of the plugins and thus can control what gets injected into them. > > Ralph > > > > > > By the way, an alternative approach for JsonTemplateLayout I had in my is > > this: > > > > 1. Make `EventResolverFactories` and each individual `EventResolver` & > > `EventResolverFactory` public. > > 2. Add a > > `JsonTemplateLayout.Builder#eventTemplateResolverFactoryMapProvider` field > > of type `String` which is pointing to a method extending from > > `Supplier<Map<String, TemplateResolverFactory<LogEvent, > > EventResolverContext, ? extends TemplateResolver<LogEvent>>>` > > 3. Set the default value of the `eventTemplateResolverFactoryMapProvider` > > to `EventResolverFactories.RESOLVER_FACTORY_BY_NAME`. > > > > So that if need arises, users can introduce their own map providers. While > > doing that, they can leverage the entire set of existing resolvers, or > > inject custom ones in isolation. (Of course the actual necessary set of > > plumbing that needs to be on our side is a little bit hairy than the 3 > > bullets I have listed above.) > > > > On Fri, Jan 29, 2021 at 7:26 PM Ralph Goers <ralph.go...@dslextreme.com> > > wrote: > > > >> My concern over this has always been that a) Log4j should not have any > >> required dependencies (and in 3.0 should only require the java base > >> module), b) The JDK itself doesn’t have dependency injection built in and > >> c) creating a large dependency injection framework in Log4j itself seems > >> like overkill. What was built to start with was the simplest thing I could > >> do that would allow users to add their own components. The plugin system > >> has been enhanced well beyond what I originally created but still isn’t > >> overly complicated. We also need to avoid Class scanning during startup as > >> much as possible as that has proven to be very slow. > >> > >
