Re: Redundant abstractions: property sources, lookups and pattern converters
Hi Ralph,
On Tue, 16 Jul 2024 at 15:17, Ralph Goers wrote:
> > * we have a `SpringLookup` and a `SpringPropertySource`.
>
> SpringLookup returns the value of a key. A property source locates a set of
> keys. So comparing them is apples to oranges. Or rather - an apple to a
> bushel of apples. PropertySources are NOT available to the logging
> configuration. However, a Lookup can certainly use a PropertySource to locate
> the keys it is to resolve.
Yes, property sources can not be addressed directly, since they are
not namespaced, as you remark below.
Other than that lookups and property sources are pretty similar: the
`forEach()` methods of property sources are only useful for caching
and I am not sure caching dozens of not Log4j related properties is
useful.
> > * we have a `${date:...}` lookup and a `%d{...}`. Note that
>
> > `$${date:...}` and `%d{...}` give the same result in a
> > `PatternLayout`!
>
> While they do in the Layout they do not have the same behavior when used in
> the FilePattern on a Rolling Appender.
Good point, I have a note about that in the revamped
RollingFileAppender description.
Although `$${date:...}` and `%d{...}` are not equivalent for
`filePattern`, I don't see a reason for users to use the former.
A file pattern like `$${date:-MM}/%d{-MM-dd}.log` that I often
see in user questions, simply doesn't do what the user wants. The
`2024-06-30.log` file ends up in the `2024-07` folder!
The user should use `%d{-MM}/%d{-MM-dd}.log`, which works
correctly since 2012 according to Git.
> IMO, it was a poor decision to use the data in the file pattern to determine
> the rollover interval.
I share the same opinion. Autodetection is very useful for simple
cases, but we might want to add a `frequency` config attribute to
allow users to override autodetection.
Same thing applies to the autodetection of compress actions: users
might want to configure those explicitly, e.g., if they want to
configure the compression level and other parameters.
> > ## Equivalent concepts in other frameworks
> >
> > Spring Boot's equivalent of both `PropertiesUtil` and `Interpolator`
> > is the single `Environment`[1] concept:
>
> However, Spring’s variables are primarily used while configuring the
> application. The same is true with PropertySources. OTOH Lookups are meant to
> be used while the application is running.
If we exclude RoutingAppender, most lookups are used at configuration
time. Less than 10 attributes accept lookups at runtime. I have
documented them all recently:
https://logging.staged.apache.org/log4j/2.x/manual/configuration.html#lazy-property-substitution
> > What do you think?
>
> I am not convinced.
>
> The advantage with Lookups is that they can be namespaced. i.e. ${ctx:myKey}.
> This is a nice short way of expressing which Map you want to look in.
> Specifying ${myKey} would require merging all the Maps together.
Namespacing can be both an advantage and a disadvantage:
* you don't want ephemeral context data to pollute the global
namespace, so `${ctx:myKey}` is necessary. However in many places
(like the `pattern` of a `Routes`) component, it could be replaced by
`%X{myKey}`.
* often users are startled by the necessity of adding a namespace,
e.g. `${sys:myKey}`, especially if the migrate from Log4j 1.
The `sys` and `env` lookups should often be used together, e.g.
`${sys:myKey:-${env:myKey}}`. This would be much simpler if we remove
the namespace.
> I am not a fan of making either of these pluggable. Making StrSubstitutor
> pluggable is a security risk. PropertiesUtil is pluggable by way of custom
> PropertySources.
It is a security risk, but it is not **our** security risk.
If we make `StrSubstitutor` pluggable and Spring decides to use its
SpEL processor to evaluate configuration attributes, it is up to them
to make sure it is secure.
The advantage of delegating string interpolation and configuration
properties to the runtime environment is that users will have a
coherent experience.
Right now SpringPropertySource is the property source with highest
priority. But, there are still other property sources, so Spring users
need to know about both `log4j2.component.properties` and
`application.properties`.
If we make `PropertiesUtil` pluggable, they can forget about the latter.
The same applies to `StrSubstitutor`: a Spring user might want to use
`${value:default}` without necessarily learning our
`${value:-default}` syntax.
And if Spring Boot maintainers decide it is safe enough, Spring users
could use `#{...}` SpEL expressions at runtime.
> Compared to a Lookup a PatternConverter is fairly expensive. A Lookup simply
> returns the value of a key from a Map. OTOH a PatternConverter is really a
> formatter. Granted we do have things like the upper Lookup that acts as a
> formatter but its capabilities are purposefully limited.
It depends on the application: if users employ `$${ctx:myKey}` in a
PatternLayout, this will be more expensive than
Re: Redundant abstractions: property sources, lookups and pattern converters
I’ll note one design constraint behind property sources and lookups. Property
sources are defined in the API while lookups are defined in Core. Besides that,
property sources are intended for use with configuration (either for components
outside the config file or for reuse in a config file) while lookups are
intended for use with log events, though the ability to use properties in log
events makes this confusing.
> On Jul 15, 2024, at 08:30, Piotr P. Karwasz wrote:
>
> Since the creation of Log4j 2, the concepts of `PropertySource` and
> `StrLookup` and the concepts of `StrLookup` and
> `LogEventPatternConverter` have strongly converged to each other.
> We might consider removing some of them in Log4j Core 4.
>
> ## Problem
>
> Log4j Core uses multiple map-like abstractions that evaluate strings:
>
> * `PropertySource`s are used to provide additional sources of
> **externalized configuration** to
> `PropertiesUtil/PropertyEnvironment`, which is used to modify default
> values of Log4j components.
> * `StrLookup`s are used by `Interpolator` to interpolate the
> attributes of the configuration file, which is something a
> `PropertiesUtil/PropertyEnvironment` could also do.
> The `Interpolator` is also used at runtime by `PatternLayout` to
> format log events.
> * `LogEventPatternConverter`s, which are used by `PatternLayout` to
> format log events.
>
> Since the domains of these abstractions overlap, we end up
> implementing these in pairs. For example:
>
> * we have a `SpringLookup` and a `SpringPropertySource`.
> * we have a `${date:...}` lookup and a `%d{...}`. Note that
> `$${date:...}` and `%d{...}` give the same result in a
> `PatternLayout`!
>
> ## Equivalent concepts in other frameworks
>
> Spring Boot's equivalent of both `PropertiesUtil` and `Interpolator`
> is the single `Environment`[1] concept:
>
> * The `Environment` provides default values of Spring Boot beans, like
> `PropertiesUtil` does.
> * `${name}` expressions can be used in XML bean definitions[2], which
> is the concept that resembles our configuration file.
>
> If `${name}` expressions are too restrictive (e.g. you can use them to
> get the current date), SpEL expressions can be used. Therefore SpEL
> expressions ressemble the way we use pattern converters.
>
> The Jakarta world also released a `Config`[3] concept, which works
> pretty much like Spring's `Environment`.
>
> ## Possible solutions
>
> For Log4j 4 we could consider:
>
> * removing the `StrLookup` concept and provide a simple `Interpolator`
> that uses `PropertiesUtil`.
> * make both `PropertiesUtil` and `StrSubstitutor` pluggable: external
> frameworks might want to replace our implementations of these
> components entirely with their own implementations.
> * enhance the few configuration attributes that accept lazy lookups to
> accept `PatternLayout` patterns instead.
>
> What do you think?
>
> [1]
> https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/core/env/Environment.html
> [2]
> https://docs.spring.io/spring-framework/reference/core/beans/dependencies/factory-properties-detailed.html
> [3]
> https://download.eclipse.org/microprofile/microprofile-config-3.0/microprofile-config-spec-3.0.html#configprovider
Re: Redundant abstractions: property sources, lookups and pattern converters
Hi Matt, On Wed, 17 Jul 2024 at 19:03, Matt Sicker wrote: > I’ll note one design constraint behind property sources and lookups. Property > sources are defined in the API while lookups are defined in Core. Since Log4j Core 3 has a different properties system, property sources are technically defined in the API, but are almost unused in the API. They are only used: 1. For the `log4j.provider` property[1], 2. For the `SimpleLoggerContext` and related classes[2]. Status logger since 2.23.0 does not depend on `PropertiesUtil`[3]. The remaining properties (including the configuration of `ThreadContextMap`) are used in Log4j Core. In this way Log4j Core 3 can have its own properties system. > Besides that, property sources are intended for use with configuration > (either for components outside the config file or for reuse in a config file) > while lookups are intended for use with log events, though the ability to use > properties in log events makes this confusing. Most lookups don't really profit from the `LogEvent` parameter[4] and those that do, have a corresponding `PatternConverter`. Piotr [1] https://logging.staged.apache.org/log4j/2.x/manual/systemproperties.html#properties-log4j-api [2] https://logging.staged.apache.org/log4j/2.x/manual/simple-logger.html#config [3] https://logging.staged.apache.org/log4j/2.x/manual/status-logger.html#properties [4] https://logging.staged.apache.org/log4j/2.x/manual/lookups.html#collection
Re: Redundant abstractions: property sources, lookups and pattern converters
> On Jul 17, 2024, at 3:09 AM, Piotr P. Karwasz wrote:
>
> Hi Ralph,
>
> On Tue, 16 Jul 2024 at 15:17, Ralph Goers wrote:
>>> * we have a `SpringLookup` and a `SpringPropertySource`.
>>
>> SpringLookup returns the value of a key. A property source locates a set of
>> keys. So comparing them is apples to oranges. Or rather - an apple to a
>> bushel of apples. PropertySources are NOT available to the logging
>> configuration. However, a Lookup can certainly use a PropertySource to
>> locate the keys it is to resolve.
>
> Yes, property sources can not be addressed directly, since they are
> not namespaced, as you remark below.
> Other than that lookups and property sources are pretty similar: the
> `forEach()` methods of property sources are only useful for caching
> and I am not sure caching dozens of not Log4j related properties is
> useful.
>
>>> * we have a `${date:...}` lookup and a `%d{...}`. Note that
>>
>>> `$${date:...}` and `%d{...}` give the same result in a
>>> `PatternLayout`!
>>
>> While they do in the Layout they do not have the same behavior when used in
>> the FilePattern on a Rolling Appender.
>
> Good point, I have a note about that in the revamped
> RollingFileAppender description.
>
> Although `$${date:...}` and `%d{...}` are not equivalent for
> `filePattern`, I don't see a reason for users to use the former.
> A file pattern like `$${date:-MM}/%d{-MM-dd}.log` that I often
> see in user questions, simply doesn't do what the user wants. The
> `2024-06-30.log` file ends up in the `2024-07` folder!
> The user should use `%d{-MM}/%d{-MM-dd}.log`, which works
> correctly since 2012 according to Git.
Really? It only has a 50% chance of being correct as only one of those can be
used to determine the rollover interval. As I said, this should be changed so
that the rollover interval is in a separate attribute.
>
>> IMO, it was a poor decision to use the data in the file pattern to determine
>> the rollover interval.
>
> I share the same opinion. Autodetection is very useful for simple
> cases, but we might want to add a `frequency` config attribute to
> allow users to override autodetection.
>
> Same thing applies to the autodetection of compress actions: users
> might want to configure those explicitly, e.g., if they want to
> configure the compression level and other parameters.
>
>>> ## Equivalent concepts in other frameworks
>>>
>>> Spring Boot's equivalent of both `PropertiesUtil` and `Interpolator`
>>> is the single `Environment`[1] concept:
>>
>> However, Spring’s variables are primarily used while configuring the
>> application. The same is true with PropertySources. OTOH Lookups are meant
>> to be used while the application is running.
>
> If we exclude RoutingAppender, most lookups are used at configuration
> time. Less than 10 attributes accept lookups at runtime. I have
> documented them all recently:
>
> https://logging.staged.apache.org/log4j/2.x/manual/configuration.html#lazy-property-substitution
Any sentence that starts with “If we exclude…” means you want to break those
things being excluded.
>
>>> What do you think?
>>
>> I am not convinced.
>>
>> The advantage with Lookups is that they can be namespaced. i.e.
>> ${ctx:myKey}. This is a nice short way of expressing which Map you want to
>> look in. Specifying ${myKey} would require merging all the Maps together.
>
> Namespacing can be both an advantage and a disadvantage:
>
> * you don't want ephemeral context data to pollute the global
> namespace, so `${ctx:myKey}` is necessary. However in many places
> (like the `pattern` of a `Routes`) component, it could be replaced by
> `%X{myKey}`.
> * often users are startled by the necessity of adding a namespace,
> e.g. `${sys:myKey}`, especially if the migrate from Log4j 1.
> The `sys` and `env` lookups should often be used together, e.g.
> `${sys:myKey:-${env:myKey}}`. This would be much simpler if we remove
> the namespace.
There are better ways to deal with this than dumping namespaces entirely. For
example, creating a lookup that is an alias for the system property lookup
followed by the environment lookup. Or a dynamic way to create a new lookup by
listing the lookups you want to include and their order.
>
>> I am not a fan of making either of these pluggable. Making StrSubstitutor
>> pluggable is a security risk. PropertiesUtil is pluggable by way of custom
>> PropertySources.
>
> It is a security risk, but it is not **our** security risk.
> If we make `StrSubstitutor` pluggable and Spring decides to use its
> SpEL processor to evaluate configuration attributes, it is up to them
> to make sure it is secure.
Uhh, no. It will be OUR security risk for opening this up to allow people to do
stupid stuff (just like we did).
>
> The advantage of delegating string interpolation and configuration
> properties to the runtime environment is that users will have a
> coherent experience.
> Right now SpringPropertySo
Re: Redundant abstractions: property sources, lookups and pattern converters
> On Jul 17, 2024, at 2:11 PM, Piotr P. Karwasz wrote: > > Hi Matt, > > On Wed, 17 Jul 2024 at 19:03, Matt Sicker wrote: >> I’ll note one design constraint behind property sources and lookups. >> Property sources are defined in the API while lookups are defined in Core. > > Since Log4j Core 3 has a different properties system, property sources > are technically defined in the API, but are almost unused in the API. > They are only used: > > 1. For the `log4j.provider` property[1], > 2. For the `SimpleLoggerContext` and related classes[2]. > > Status logger since 2.23.0 does not depend on `PropertiesUtil`[3]. > The remaining properties (including the configuration of > `ThreadContextMap`) are used in Log4j Core. In this way Log4j Core 3 > can have its own properties system. > >> Besides that, property sources are intended for use with configuration >> (either for components outside the config file or for reuse in a config >> file) while lookups are intended for use with log events, though the ability >> to use properties in log events makes this confusing. > > Most lookups don't really profit from the `LogEvent` parameter[4] and > those that do, have a corresponding `PatternConverter`. Conversely, most PatternConverters are only useful in a log event. Allowing them to be used outside of one would be awful. The only exception that comes to mind is the DateConverter. Ralph
