a) I understand the policy that core shouldn't have any dependencies.
b) Maybe `ServiceLoader`s?
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.
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.
>
> Ralph
>
> > On Jan 29, 2021, at 10:28 AM, Matt Sicker <boa...@gmail.com> wrote:
> >
> > That sounds like one of the main goals of the DI rewrite I started. To
> > give more context on the existing system works, the ability to inject
> > complex objects into plugins is primarily used for mapping nested
> > configurations. Basically, anything corresponding to an XML element
> > was injectable into other elements corresponding to the configuration
> > file structure. Not every component in Log4j is integrated into the
> > system yet, so there are some pain points like this in 2.x which can
> > be seen in some things being configurable via system properties while
> > others have custom plugin elements defined for users to define their
> > configuration inside. Some of these testability concerns were likely
> > missed because their underlying code was fast enough to not need
> > mocks.
> >
> > Now if you'd like to inject objects instantiated from some arbitrary
> > point similar to Spring/Guice/etc., that will require a larger
> > overhaul (likely by rebooting my mean-bean-machine branch at some
> > point), but if your needs are already structured in a tree, then
> > @PluginElement is the equivalent of @Inject, while @PluginFactory is
> > equivalent to @Produces. Depending on your use case, it could be
> > supportable with the existing plugin system, or it may require
> > revisiting the DI rewrite, especially if there's wider interest in
> > completing that.
> >
> > On Fri, 29 Jan 2021 at 06:56, Volkan Yazıcı <volkan.yaz...@gmail.com>
> wrote:
> >>
> >> Hello,
> >>
> >> I am doing some investigation on how to replace statically bound
> >> JsonTemplateLayout resolvers with plugins. For inspiration I was
> looking at
> >> PatternLayout and found the following: PatternLayout calls PatternParser
> >> ctor, which performs the following:
> >>
> >> final PluginManager manager = new PluginManager(converterKey);
> >> manager.collectPlugins(config == null ? null :
> config.getPluginPackages());
> >> final Map<String, PluginType<?>> plugins = manager.getPlugins();
> >> final Map<String, Class<PatternConverter>> converters = new
> >> LinkedHashMap<>();
> >>
> >> for (final PluginType<?> type : plugins.values()) {
> >> try {
> >> @SuppressWarnings("unchecked")
> >> final Class<PatternConverter> clazz = (Class<PatternConverter>)
> >> type.getPluginClass();
> >> if (filterClass != null && !filterClass.isAssignableFrom(clazz))
> {
> >> continue;
> >> }
> >>
> >>
> >> This got me really puzzled. I was expecting matched plugins to be
> injected
> >> into the @PluginFactory ctor of PatternLayout as a parameter of type
> >> List<PatternConverter> like every other DI system out there. That is,
> >>
> >> @PluginFactory
> >> AwesomeLayout(
> >> @PluginConfiguration Configuration config,
> >> @PluginElement("converters") List<PatternConverter> converters) {
> >> // ...
> >> }
> >>
> >>
> >> I find usage of PluginManager::new an anti-pattern. For one, it is not
> >> possible to unit test Pattern{Layout,Parser} by injecting only an
> isolated
> >> set of converters. Second, it is not possible to mock PluginManager
> either.
> >>
> >> Is injection of plugins extending a certain interface/class as a
> >> @PluginFactory ctor parameter something we can introduce to the plugin
> >> system?
> >>
> >> Kind regards.
> >
>
>
>
