[
https://issues.apache.org/jira/browse/LOG4J2-952?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14718085#comment-14718085
]
Gary Gregory commented on LOG4J2-952:
-------------------------------------
Ralph,
Nice work!
Great stuff!
Here are my note; hopefully taken as well-intentioned, respectful and
constructive criticism. (It's tricky when not in person sometimes!)
In the text below, I tried to sort from most to least important but it's all
important really :-)
----
Right now, it looks like we have two ways to use an assembler: Through a
properties file (which uses a {{ConfigurationFactory}}) or a programatically
with a custom {{ConfigurationFactory}}.
For programmatic configuration, being forced to write a custom
{{ConfigurationFactory}} feels too twisty to me.
I should just be able to write my assembler code (pun not intended, more on the
name later) and feed it to the {{Configurator}}. Yes, I can still do it with a
{{ConfigurationFactory}}, which seems nice for other use cases, like the
properties file case.
This feels more natural:
{code:java}
Configuation configuation = assembler.assemble();
Configurator.initialize(classloader, configuation);
{code}
How do we get the assembler is for later in this text.
We already have {{Configurator.initialize(ClassLoader, ConfigurationSource)}}.
If {{ConfigurationSource}} supports {{Assembler}}, then I could say:
{code:java}
Configurator.initialize(ClassLoader, new ConfigurationSource(assembler));
{code}
And we do not need {{Configurator.initialize(classloader, configuation)}}
except for convenience.
We then need an {{AssemblerConfigurationFactory}} that extends
{{ConfigurationFactory}}.
Back to the question of where Assemblers come from; the patch provides:
- ConfigurationFactory.getConfigurationAssembler()
- ConfigurationFactory.getConfigurationAssembler(Class<T>)
But this *really* feels in the wrong place to me. A {{ConfigurationFactory}}
should return {{Configuration}}s, but since an {{Assembler}} _produces_
{{Configuration}}s, then maybe it's OK. I'm not 100% sold on that though.
Having an {{AssemblerFactory}} seems like the obvious answer. This then let's
me write code that steers clear of {{ConfigurationFactory}}:
{code:java}
Assembler assembler = AssemblerFactory.getConfigurationAssembler();
// ... build up assembler ...
Configuation configuation = assembler.assemble();
Configurator.initialize(ClassLoader, new ConfigurationSource(assembler));
{code}
----
I'm not crazy about the term "assembler". We are not using "builder" because...
it's used to build plugin instances here and there? That does not seem like
strong enough a reason.
Looking back at the 'gang of four' book, "Builder" really fits what we are
doing here. I looked at the other Creational Pattern and the Abstract Factory
could fit the bill too.
It's just that I've not seen the term 'assembler' used like this before and it
feels weird and violates (for me) the Principle of Least Astonishment.
I feel like I need to explain to users why it's am assembler and not a builder
or a factory. We should not have to do that or cause users to think about it.
Going back to the code, the {{Assembler}} class is exactly like the class
{{org.apache.logging.log4j.core.util.Builder}}, so that's lame (IMO), we should
at least reuse {{Builder}}. It would be (XP) smelly not to.
I'm starting to feel "builder" over "assembler" more and more but I do not know
of your full reasoning so I'll stop here and let you reply.
----
The test has code like:
{code:java}
assembler.addAppender(appenderAssembler.assemble());
{code}
The assemble() method is called all over the place. That does not feel right.
It's surfaces too many of the mechanics. Why not have a method
{{ConfigurationAssembler.addAppender(AppenderAssembler)}} that does the
assemble on my behalf?
In fact, I claim that we can have a more generic method:
{{ConfigurationAssembler.add(Assembler)}} that does the right thing based on
the argument. The patch has:
{code:java}
public Configuration getConfiguration(final String name, final URI
configLocation) {
ConfigurationAssembler<AssembledConfiguration> assembler =
getConfigurationAssembler();
assembler.setStatusLevel(Level.ERROR);
assembler.addFilter(
assembler.getFilterAssembler("ThresholdFilter", "ACCEPT",
"NEUTRAL").addAttribute("level", "DEBUG")
.assemble());
AppenderAssembler appenderAssembler =
assembler.getAppenderAssembler("Stdout", "CONSOLE").addAttribute("target",
"SYSTEM_OUT");
appenderAssembler.addLayout(assembler.getLayoutAssembler("PatternLayout").
addAttribute("pattern", "%d [%t] %-5level:
%msg%n%throwable").assemble());
appenderAssembler.addFilter(
assembler.getFilterAssembler("MarkerFilter", "DENY",
"NEUTRAL").addAttribute("marker", "FLOW").
assemble());
assembler.addAppender(appenderAssembler.assemble());
assembler.addLogger(assembler.getLoggerAssembler("org.apache.logging.log4j",
"DEBUG").
addAppenderRef(assembler.getAppenderRefAssembler("Stdout").assemble()).
addAttribute("additivity", "false").assemble());
assembler.addRootLogger(assembler.getRootLoggerAssembler("ERROR")
.addAppenderRef(assembler.getAppenderRefAssembler("Stdout").assemble()).assemble());
return assembler.assemble();
}
{code}
Just {{addAppender(AppenderAssembler)}} and similar methods would give us:
{code:java}
public Configuration getConfiguration(final String name, final URI
configLocation) {
ConfigurationAssembler<AssembledConfiguration> assembler =
getConfigurationAssembler();
assembler.setStatusLevel(Level.ERROR);
assembler.addFilter(
assembler.getFilterAssembler("ThresholdFilter", "ACCEPT",
"NEUTRAL").addAttribute("level", "DEBUG"));
AppenderAssembler appenderAssembler =
assembler.getAppenderAssembler("Stdout", "CONSOLE").addAttribute("target",
"SYSTEM_OUT");
appenderAssembler.addLayout(assembler.getLayoutAssembler("PatternLayout").
addAttribute("pattern", "%d [%t] %-5level: %msg%n%throwable"));
appenderAssembler.addFilter(
assembler.getFilterAssembler("MarkerFilter", "DENY",
"NEUTRAL").addAttribute("marker", "FLOW"));
assembler.addAppender(appenderAssembler);
assembler.addLogger(assembler.getLoggerAssembler("org.apache.logging.log4j",
"DEBUG").
addAppenderRef(assembler.getAppenderRefAssembler("Stdout")).
addAttribute("additivity", "false"));
assembler.addRootLogger(assembler.getRootLoggerAssembler("ERROR")
.addAppenderRef(assembler.getAppenderRefAssembler("Stdout")));
return assembler.assemble();
}
{code}
Seing code that first calls {{getFilterAssembler()}}, then {{addFilter()}}
seems redundant; I built a filter, it's typed as such, now I want to add it,
but do I really have to say "add filter" vs. just "add"?
A smart {{add()}} would give us:
{code:java}
public Configuration getConfiguration(final String name, final URI
configLocation) {
ConfigurationAssembler<AssembledConfiguration> assembler =
getConfigurationAssembler();
assembler.setStatusLevel(Level.ERROR);
assembler.add(
assembler.getFilterAssembler("ThresholdFilter", "ACCEPT",
"NEUTRAL").addAttribute("level", "DEBUG"));
AppenderAssembler appenderAssembler =
assembler.getAppenderAssembler("Stdout", "CONSOLE").addAttribute("target",
"SYSTEM_OUT");
appenderAssembler.add(assembler.getLayoutAssembler("PatternLayout").
addAttribute("pattern", "%d [%t] %-5level: %msg%n%throwable"));
appenderAssembler.add(
assembler.getFilterAssembler("MarkerFilter", "DENY",
"NEUTRAL").addAttribute("marker", "FLOW"));
assembler.add(appenderAssembler);
assembler.add(assembler.getLoggerAssembler("org.apache.logging.log4j",
"DEBUG").
add(assembler.getAppenderRefAssembler("Stdout")).
addAttribute("additivity", "false"));
assembler.addRootLogger(assembler.getRootLoggerAssembler("ERROR")
.add(assembler.getAppenderRefAssembler("Stdout")));
return assembler.assemble();
}
{code}
Pushing it further, since I am talking to an assembler, do I really need to say
{{getFilterAssembler()}} vs. {{getFilter}}. These get APIs only returns
assemblers, there cannot be any confusion. So, removing the "Assembler" suffix,
we have:
{code:java}
public Configuration getConfiguration(final String name, final URI
configLocation) {
ConfigurationAssembler<AssembledConfiguration> assembler =
getConfigurationAssembler();
assembler.setStatusLevel(Level.ERROR);
assembler.add(
assembler.getFilter("ThresholdFilter", "ACCEPT",
"NEUTRAL").addAttribute("level", "DEBUG"));
AppenderAssembler appenderAssembler = assembler.getAppender("Stdout",
"CONSOLE").addAttribute("target", "SYSTEM_OUT");
appenderAssembler.add(assembler.getLayout("PatternLayout").
addAttribute("pattern", "%d [%t] %-5level: %msg%n%throwable"));
appenderAssembler.add(
assembler.getFilter("MarkerFilter", "DENY",
"NEUTRAL").addAttribute("marker", "FLOW"));
assembler.add(appenderAssembler);
assembler.add(assembler.getLogger("org.apache.logging.log4j", "DEBUG").
add(assembler.getAppenderRef("Stdout")).
addAttribute("additivity", "false"));
assembler.addRootLogger(assembler.getRootLogger("ERROR")
.add(assembler.getAppenderRef("Stdout")));
return assembler.assemble();
}
{code}
Better?
The "get" prefix is another one I am not sure about, because these are all
factory methods, so "new" would be better. Unless these can be cached?
With new:
{code:java}
public Configuration getConfiguration(final String name, final URI
configLocation) {
ConfigurationAssembler<AssembledConfiguration> assembler =
getConfigurationAssembler();
assembler.setStatusLevel(Level.ERROR);
assembler.add(
assembler.newFilter("ThresholdFilter", "ACCEPT",
"NEUTRAL").addAttribute("level", "DEBUG"));
AppenderAssembler appenderAssembler = assembler.newAppender("Stdout",
"CONSOLE").addAttribute("target", "SYSTEM_OUT");
appenderAssembler.add(assembler.newLayout("PatternLayout").
addAttribute("pattern", "%d [%t] %-5level: %msg%n%throwable"));
appenderAssembler.add(
assembler.newFilter("MarkerFilter", "DENY",
"NEUTRAL").addAttribute("marker", "FLOW"));
assembler.add(appenderAssembler);
assembler.add(assembler.newLogger("org.apache.logging.log4j", "DEBUG").
add(assembler.newAppenderRef("Stdout")).
addAttribute("additivity", "false"));
assembler.addRootLogger(assembler.newRootLogger("ERROR")
.add(assembler.newAppenderRef("Stdout")));
return assembler.assemble();
}
{code}
Better?
----
In the test I see:
{code:java}
AppenderAssembler appenderAssembler =
assembler.getAppenderAssembler("Stdout", "CONSOLE").addAttribute("target",
"SYSTEM_OUT");
{code}
And the Javadoc says:
{code:java}
/**
* Returns an Assembler for creating Appenders.
* @param name The name of the Appender.
* @param type The Plugin type of the Appender.
* @return the AppenderAssembler.
*/
AppenderAssembler getAppenderAssembler(String name, String type);
{code}
But the only way I see this of matching up is if the "type" argument matches
the {{ConsoleAppeder}}'s {{@Plugin name}}.
If that is indeed the case, the parameter name should be changed to
"pluginName". That makes it obvious and connects the dots for the reader.
This comment applies to other APIs.
----
What is the argument _against_ having:
{code:java}
FilterAssembler getFilterAssembler(String type, Filter.Result onMatch,
Filter.Result onMisMatch);
{code}
This could be an _additional_ API, or replace the current String-based API, not
sure.
Same idea for APIs that take String levels.
----
In
{{org.apache.logging.log4j.core.config.ConfigurationFactory.getConfigurationAssembler(Class<T>)}},
this is redundant:
{code:java}
return new DefaultConfigurationAssembler<T>(clazz);
{code}
Instead, say:
{code:java}
return new DefaultConfigurationAssembler<>(clazz);
{code}
Note the use of Java 7 diamond notation.
There are other generics clean ups we can do later, once other issues are
settled.
----
org.apache.logging.log4j.core.config.assembler.api
org.apache.logging.log4j.core.config.assembler.impl
If there are two packages then the {{impl}} classes must be public. If you put
all types in the same package, you can make the impl classes package private.
One less worry about people using stuff they're not supposed to. Unless you
*mean* for the impl classes to be publicly used.
----
> FAQ: How do I configure log4j2 programmatically in code without a
> configuration file?
> -------------------------------------------------------------------------------------
>
> Key: LOG4J2-952
> URL: https://issues.apache.org/jira/browse/LOG4J2-952
> Project: Log4j 2
> Issue Type: Bug
> Components: API, Configurators, Documentation
> Affects Versions: 2.1
> Reporter: Joe Merten
> Attachments: LOG4J2-952-2.patch, LOG4J2-952-3.patch,
> LOG4J2-952-4.patch, LOG4J2-952.patch
>
>
> I found [this
> link|http://logging.apache.org/log4j/2.x/faq.html#config_from_code] which
> said:
> {quote}
> You could use the static method #initialize(String contextName, ClassLoader
> loader, String configLocation) in
> org.apache.logging.log4j.core.config.Configurator. (You can pass null for the
> class loader.) Be aware that this class is not part of the public API so your
> code may break with any minor release.
> {quote}
> This documentation is unclear because it points to a member function which
> needs a filename {{configLocation}} where as the topic is »without a
> configuration file«.
> It shoud rather point to the member function
> {{org.apache.logging.log4j.core.config.Configurator.initialize(ClassLoader
> loader, ConfigurationSource source)}}.
> Example:
> {code:java}
> import org.apache.logging.log4j.core.config.ConfigurationSource;
> import org.apache.logging.log4j.core.config.Configurator;
> final String hardCodedXmlConfig =
> "<?xml version='1.0' encoding='UTF-8'?>\n" +
> "<Configuration status='INFO'>\n" +
> " <Appenders>\n" +
> " <Console name='Console' target='SYSTEM_OUT'>\n" +
> " <PatternLayout pattern='%d{HH:mm:ss.SSS} [%t] %-5level
> %logger{36} - %msg%n'/>\n" +
> " </Console>\n" +
> " </Appenders>\n" +
> " <Loggers>\n" +
> " <Root level='debug'>\n" +
> " <AppenderRef ref='Console'/>\n" +
> " </Root>\n" +
> " </Loggers>\n" +
> "</Configuration>\n";
> try {
> Configurator.initialize(null, new ConfigurationSource(new
> ByteArrayInputStream(hardCodedXmlConfig.getBytes())));
> } catch (IOException e) {
> e.printStackTrace();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
