Injecting matching plugins to ctor as a parameter

[Volkan Yazıcı]

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> plugins = manager.getPlugins();
final Map> converters = new
LinkedHashMap<>();

for (final PluginType type : plugins.values()) {
try {
@SuppressWarnings("unchecked")
final Class clazz = (Class)
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 like every other DI system out there. That is,

@PluginFactory
AwesomeLayout(
@PluginConfiguration Configuration config,
@PluginElement("converters") List 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.

Renaming log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy (Was: [jira] LOG4J2-3009)

[Volkan Yazıcı]

Triggered by Carter's remark... Shall we
rename log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy
and display a warning if the deprecated one is defined? Are there more of
such properties?

On Fri, Jan 29, 2021 at 4:44 PM Carter Kozak (Jira)  wrote:

> The {{log4j2.asyncLoggerThreadNameStrategy}} may be set to {{UNCACHED}}
> to override this default on all systems. It may be set in a
> log4j2.component.properties file, or via a jvm system property.
> It's a tad odd that these are described as properties for async logging,
> in reality they're more closely associated with gc-logging, and don't
> really relate to asynchronous features. Likely a historical artifact.
> Docs:
>
> https://logging.apache.org/log4j/2.x/manual/configuration.html#SystemProperties
> https://logging.apache.org/log4j/2.x/manual/async.html#SysPropsAllAsync
>

Re: Renaming log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy (Was: [jira] LOG4J2-3009)

[Carter Kozak]

We have a framework in place to migrate property names without disrupting 
users, I would support using a default name that aligns more closely with the 
feature. I don't think we warn when deprecated names are used, I'd expect that 
could become noisy, and we want folks to upgrade without worrying about scaring 
their users with warnings from us.

For what it's worth, the feature is entirely unnecessary beyond java 8 since 
the getName() allocations were removed.

Carter

On Fri, Jan 29, 2021, at 10:53, Volkan Yazıcı wrote:
> Triggered by Carter's remark... Shall we
> rename log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy
> and display a warning if the deprecated one is defined? Are there more of
> such properties?
> 
> On Fri, Jan 29, 2021 at 4:44 PM Carter Kozak (Jira)  wrote:
> 
> > The {{log4j2.asyncLoggerThreadNameStrategy}} may be set to {{UNCACHED}}
> > to override this default on all systems. It may be set in a
> > log4j2.component.properties file, or via a jvm system property.
> > It's a tad odd that these are described as properties for async logging,
> > in reality they're more closely associated with gc-logging, and don't
> > really relate to asynchronous features. Likely a historical artifact.
> > Docs:
> >
> > https://logging.apache.org/log4j/2.x/manual/configuration.html#SystemProperties
> > https://logging.apache.org/log4j/2.x/manual/async.html#SysPropsAllAsync
> >
> 

Re: Renaming log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy (Was: [jira] LOG4J2-3009)

[Volkan Yazıcı]

"a framework in place to migrate property names without disrupting users"?
That is totally new to me. Any pointers?

On Fri, Jan 29, 2021 at 5:01 PM Carter Kozak  wrote:

> We have a framework in place to migrate property names without disrupting
> users, I would support using a default name that aligns more closely with
> the feature. I don't think we warn when deprecated names are used, I'd
> expect that could become noisy, and we want folks to upgrade without
> worrying about scaring their users with warnings from us.
>
> For what it's worth, the feature is entirely unnecessary beyond java 8
> since the getName() allocations were removed.
>
> Carter
>
> On Fri, Jan 29, 2021, at 10:53, Volkan Yazıcı wrote:
> > Triggered by Carter's remark... Shall we
> > rename log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy
> > and display a warning if the deprecated one is defined? Are there more of
> > such properties?
> >
> > On Fri, Jan 29, 2021 at 4:44 PM Carter Kozak (Jira) 
> wrote:
> >
> > > The {{log4j2.asyncLoggerThreadNameStrategy}} may be set to {{UNCACHED}}
> > > to override this default on all systems. It may be set in a
> > > log4j2.component.properties file, or via a jvm system property.
> > > It's a tad odd that these are described as properties for async
> logging,
> > > in reality they're more closely associated with gc-logging, and don't
> > > really relate to asynchronous features. Likely a historical artifact.
> > > Docs:
> > >
> > >
> https://logging.apache.org/log4j/2.x/manual/configuration.html#SystemProperties
> > >
> https://logging.apache.org/log4j/2.x/manual/async.html#SysPropsAllAsync
> > >
> >
>

Re: Renaming log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy (Was: [jira] LOG4J2-3009)

[Carter Kozak]

Oh hmm, it looks like things are a little more manual than I'd thought! 
PropertiesUtil.Util.tokenize and PropertySource.getNormalForm massage the data, 
but I had thought there were direct string replacements for a few property 
names that I can't seem to find (perhaps I imagined them?). Given we already do 
some key-manipulation, I think it's a reasonable place to define mappings from 
old to new key names.

On Fri, Jan 29, 2021, at 11:10, Volkan Yazıcı wrote:
> "a framework in place to migrate property names without disrupting users"?
> That is totally new to me. Any pointers?
> 
> On Fri, Jan 29, 2021 at 5:01 PM Carter Kozak  wrote:
> 
> > We have a framework in place to migrate property names without disrupting
> > users, I would support using a default name that aligns more closely with
> > the feature. I don't think we warn when deprecated names are used, I'd
> > expect that could become noisy, and we want folks to upgrade without
> > worrying about scaring their users with warnings from us.
> >
> > For what it's worth, the feature is entirely unnecessary beyond java 8
> > since the getName() allocations were removed.
> >
> > Carter
> >
> > On Fri, Jan 29, 2021, at 10:53, Volkan Yazıcı wrote:
> > > Triggered by Carter's remark... Shall we
> > > rename log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy
> > > and display a warning if the deprecated one is defined? Are there more of
> > > such properties?
> > >
> > > On Fri, Jan 29, 2021 at 4:44 PM Carter Kozak (Jira) 
> > wrote:
> > >
> > > > The {{log4j2.asyncLoggerThreadNameStrategy}} may be set to {{UNCACHED}}
> > > > to override this default on all systems. It may be set in a
> > > > log4j2.component.properties file, or via a jvm system property.
> > > > It's a tad odd that these are described as properties for async
> > logging,
> > > > in reality they're more closely associated with gc-logging, and don't
> > > > really relate to asynchronous features. Likely a historical artifact.
> > > > Docs:
> > > >
> > > >
> > https://logging.apache.org/log4j/2.x/manual/configuration.html#SystemProperties
> > > >
> > https://logging.apache.org/log4j/2.x/manual/async.html#SysPropsAllAsync
> > > >
> > >
> >
> 

Re: Renaming log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy (Was: [jira] LOG4J2-3009)

[Matt Sicker]

Plugins have aliases via the @PluginAlias annotation, though I don't
think there's an equivalent feature for properties yet. This sounds
like something that could potentially be improved in PropertiesUtil.

On Fri, 29 Jan 2021 at 10:24, Carter Kozak  wrote:
>
> Oh hmm, it looks like things are a little more manual than I'd thought! 
> PropertiesUtil.Util.tokenize and PropertySource.getNormalForm massage the 
> data, but I had thought there were direct string replacements for a few 
> property names that I can't seem to find (perhaps I imagined them?). Given we 
> already do some key-manipulation, I think it's a reasonable place to define 
> mappings from old to new key names.
>
> On Fri, Jan 29, 2021, at 11:10, Volkan Yazıcı wrote:
> > "a framework in place to migrate property names without disrupting users"?
> > That is totally new to me. Any pointers?
> >
> > On Fri, Jan 29, 2021 at 5:01 PM Carter Kozak  wrote:
> >
> > > We have a framework in place to migrate property names without disrupting
> > > users, I would support using a default name that aligns more closely with
> > > the feature. I don't think we warn when deprecated names are used, I'd
> > > expect that could become noisy, and we want folks to upgrade without
> > > worrying about scaring their users with warnings from us.
> > >
> > > For what it's worth, the feature is entirely unnecessary beyond java 8
> > > since the getName() allocations were removed.
> > >
> > > Carter
> > >
> > > On Fri, Jan 29, 2021, at 10:53, Volkan Yazıcı wrote:
> > > > Triggered by Carter's remark... Shall we
> > > > rename log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy
> > > > and display a warning if the deprecated one is defined? Are there more 
> > > > of
> > > > such properties?
> > > >
> > > > On Fri, Jan 29, 2021 at 4:44 PM Carter Kozak (Jira) 
> > > wrote:
> > > >
> > > > > The {{log4j2.asyncLoggerThreadNameStrategy}} may be set to 
> > > > > {{UNCACHED}}
> > > > > to override this default on all systems. It may be set in a
> > > > > log4j2.component.properties file, or via a jvm system property.
> > > > > It's a tad odd that these are described as properties for async
> > > logging,
> > > > > in reality they're more closely associated with gc-logging, and don't
> > > > > really relate to asynchronous features. Likely a historical artifact.
> > > > > Docs:
> > > > >
> > > > >
> > > https://logging.apache.org/log4j/2.x/manual/configuration.html#SystemProperties
> > > > >
> > > https://logging.apache.org/log4j/2.x/manual/async.html#SysPropsAllAsync
> > > > >
> > > >
> > >
> >

Re: Injecting matching plugins to ctor as a parameter

[Matt Sicker]

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ı  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> plugins = manager.getPlugins();
> final Map> converters = new
> LinkedHashMap<>();
>
> for (final PluginType type : plugins.values()) {
> try {
> @SuppressWarnings("unchecked")
> final Class clazz = (Class)
> 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 like every other DI system out there. That is,
>
> @PluginFactory
> AwesomeLayout(
> @PluginConfiguration Configuration config,
> @PluginElement("converters") List 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.

Re: Injecting matching plugins to ctor as a parameter

[Ralph Goers]

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  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ı  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> plugins = manager.getPlugins();
>> final Map> converters = new
>> LinkedHashMap<>();
>> 
>> for (final PluginType type : plugins.values()) {
>>try {
>>@SuppressWarnings("unchecked")
>>final Class clazz = (Class)
>> 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 like every other DI system out there. That is,
>> 
>> @PluginFactory
>> AwesomeLayout(
>>@PluginConfiguration Configuration config,
>>@PluginElement("converters") List 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.
> 

Re: Renaming log4j2.asyncLoggerThreadNameStrategy to log4j2.threadNameStrategy (Was: [jira] LOG4J2-3009)

[Volkan Yazıcı]

Allright. Created LOG4J2-3011
: "Support aliases in
PropertiesUtil to ease renaming deprecated properties with new ones"

On Fri, Jan 29, 2021 at 6:20 PM Matt Sicker  wrote:

> Plugins have aliases via the @PluginAlias annotation, though I don't
> think there's an equivalent feature for properties yet. This sounds
> like something that could potentially be improved in PropertiesUtil.
>
> On Fri, 29 Jan 2021 at 10:24, Carter Kozak  wrote:
> >
> > Oh hmm, it looks like things are a little more manual than I'd thought!
> PropertiesUtil.Util.tokenize and PropertySource.getNormalForm massage the
> data, but I had thought there were direct string replacements for a few
> property names that I can't seem to find (perhaps I imagined them?). Given
> we already do some key-manipulation, I think it's a reasonable place to
> define mappings from old to new key names.
> >
> > On Fri, Jan 29, 2021, at 11:10, Volkan Yazıcı wrote:
> > > "a framework in place to migrate property names without disrupting
> users"?
> > > That is totally new to me. Any pointers?
> > >
> > > On Fri, Jan 29, 2021 at 5:01 PM Carter Kozak 
> wrote:
> > >
> > > > We have a framework in place to migrate property names without
> disrupting
> > > > users, I would support using a default name that aligns more closely
> with
> > > > the feature. I don't think we warn when deprecated names are used,
> I'd
> > > > expect that could become noisy, and we want folks to upgrade without
> > > > worrying about scaring their users with warnings from us.
> > > >
> > > > For what it's worth, the feature is entirely unnecessary beyond java
> 8
> > > > since the getName() allocations were removed.
> > > >
> > > > Carter
> > > >
> > > > On Fri, Jan 29, 2021, at 10:53, Volkan Yazıcı wrote:
> > > > > Triggered by Carter's remark... Shall we
> > > > > rename log4j2.asyncLoggerThreadNameStrategy to
> log4j2.threadNameStrategy
> > > > > and display a warning if the deprecated one is defined? Are there
> more of
> > > > > such properties?
> > > > >
> > > > > On Fri, Jan 29, 2021 at 4:44 PM Carter Kozak (Jira) <
> j...@apache.org>
> > > > wrote:
> > > > >
> > > > > > The {{log4j2.asyncLoggerThreadNameStrategy}} may be set to
> {{UNCACHED}}
> > > > > > to override this default on all systems. It may be set in a
> > > > > > log4j2.component.properties file, or via a jvm system property.
> > > > > > It's a tad odd that these are described as properties for async
> > > > logging,
> > > > > > in reality they're more closely associated with gc-logging, and
> don't
> > > > > > really relate to asynchronous features. Likely a historical
> artifact.
> > > > > > Docs:
> > > > > >
> > > > > >
> > > >
> https://logging.apache.org/log4j/2.x/manual/configuration.html#SystemProperties
> > > > > >
> > > >
> https://logging.apache.org/log4j/2.x/manual/async.html#SysPropsAllAsync
> > > > > >
> > > > >
> > > >
> > >
>

Re: Injecting matching plugins to ctor as a parameter

[Volkan Yazıcı]

Thanks so much for the prompt reply Matt.

What do you mean by "if your needs are already structured in a tree"?

I was considering having an `eventTemplateResolvers` parameter of type
`Set` in `JsonTemplateLayout.Builder` that is auto-magically
injected by the plugin system. In its current state, all available
resolvers are under the `resolver` package, which is at the same level as
`JsonTemplateLayout`. That said, users should be able to provide their own
implementations too – at the end of the day, that is the whole point of the
plugin system. Is this something doable right now? If it requires an
overhauling to the system, how can I incentivize it for you?

On Fri, Jan 29, 2021 at 6:28 PM Matt Sicker  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ı 
> 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> plugins = manager.getPlugins();
> > final Map> converters = new
> > LinkedHashMap<>();
> >
> > for (final PluginType type : plugins.values()) {
> > try {
> > @SuppressWarnings("unchecked")
> > final Class clazz = (Class)
> > 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 like every other DI system out there. That is,
> >
> > @PluginFactory
> > AwesomeLayout(
> > @PluginConfiguration Configuration config,
> > @PluginElement("converters") List 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.
>

Re: Injecting matching plugins to ctor as a parameter

[Volkan Yazıcı]

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 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>>`
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 
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  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ı 
> 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> plugins = manager.getPlugins();
> >> final Map> converters = new
> >> LinkedHashMap<>();
> >>
> >> for (final PluginType type : plugins.values()) {
> >>try {
> >>@SuppressWarnings("unchecked")
> >>final Class clazz = (Class)
> >> 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 like every other DI system out there. That is,
> >>
> >> @PluginFactory
> >> AwesomeLayout(
> >>@PluginConfiguration Configuration config,
> >>@PluginElement("converters") List 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 injectio

Re: Injecting matching plugins to ctor as a parameter

[Matt Sicker]

Another potential option I just remembered is to adopt the plugin
system as it exists to define a new elementType value. An example of
something that has only half-way integrated this way are the various
converter keys for pattern layout.

As for a tree structure, I mean that the plugin system is mostly
structured around transforming a tree of nodes (originally XML data,
but it's any configuration format) into the runtime graph of objects.
As such, it's almost like Spring's XML minus the ability to reference
the beans you instantiate other than in specific use cases like
AppenderRef. I think this limitation has led to a bit of a mess over
time with a weird in-between state where some objects might want to be
configurable or extensible in ways that don't fit this original
pattern.

On the dependency-free side, that's the idea behind the rewrite I
started. I think part of what lost momentum was trying to incorporate
support for the existing plugin system syntax while including it into
a broader and more consistent DI system. The goal of that DI system
was to be as minimal as necessary for our use case so that we don't
need any dependencies or unnecessary features. I had initially looked
into how to potentially generate code from the annotation processor
rather than probing objects at runtime via reflection, though I was
still merging the models together before I could experiment with that
idea.

For some help, if you're familiar with how CDI works, perhaps you can
help figure out a way to model the LoggerContext as a sort of scoped
context similar to session-scope and request-scope. I believe this was
one of the key integration points I was trying to reach before getting
derailed. My previous work is still in this branch, though I haven't
merged back from master in a while:
https://github.com/apache/logging-log4j2/tree/mean-bean-machine

In particular, the API and SPI structure I was working on is here:
https://github.com/apache/logging-log4j2/tree/mean-bean-machine/log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/api
https://github.com/apache/logging-log4j2/tree/mean-bean-machine/log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/spi

And the reflection-based default implementation:
https://github.com/apache/logging-log4j2/tree/mean-bean-machine/log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/defaults

Note related to ServiceLoader that I haven't quite gotten to that
point, but I'd like this to ideally only load bean metadata for things
referenced in the config file so we can lazy-load classes to avoid
long startup times. In fact, this entire bean system is detached from
the rest of the codebase still as I was still working on defining the
scoping rules for things.

It's been a while since I was working on this code, but it's mostly
based on extracting the core logic from Weld, OpenWebBeans, and Guice,
following an API that's a sort of simplified hybrid of the two.

On Fri, 29 Jan 2021 at 13:15, Volkan Yazıcı  wrote:
>
> 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 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 EventResolverContext, ? extends TemplateResolver>>`
> 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 
> 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 s