Re: Monitoring discards from async logging

[Piotr P. Karwasz]

Hi Volkan,

On Thu, 11 Apr 2024 at 21:26, Volkan Yazıcı  wrote:
> *Slightly longer answer:* Log4j has never had a holistic observability view
> to its internals. We need something new (if not, from scratch), and
> preferably, applicable to not only async. logging, but all components
> wherever this can be needed. This is time consuming hard work.
> Piotr and I, sort of, the only active Log4j maintainers nowadays, are
> swamped with other priorities. All that said, you can contract a maintainer
>  to get this
> rolling.

I think you meant "you can contribute the functionality or contract
**any** developer to do it". ;-) The way you wrote it sounds horrible.

Adam, to be entirely clear:

* Volkan and I are **not** the only maintainers. If a majority of
maintainers support your PR, personally I will not block it, although
I prefer to keep implementation details encapsulated as much as
possible, so we can refactor them at any time,

* We would like to have a holistic solution to instrumenting Log4j
Core. Even though Volkan and I undeniably work a lot on Log4j as part
of our job, adding metrics to Log4j is not covered by our clients. So
we cannot afford to deliver it ourselves in our spare time,

* Volkan probably meant that a Log4j maintainer could deliver this
work faster, but we really don't care who contributes it. If you can
contribute such a PR, we would be happy to review it. However before
starting such a major endeavour you should either discuss the details
on this mailing list or you can contact us directly and we can
organize a video call with the rest of the PMC,

* Let me stress that we treat all PRs equally, whether coming from a
project founder, PMC member or external contributors. My
co-maintainers could probably tell you more about it. ;-)

Piotr

Better Javadoc/Impl for org.apache.logging.log4j.core.Layout.getContentType()

[Gary D. Gregory]

Hi All,

Our current Javadoc says:

/**
 * Returns the content type output by this layout. The base class returns 
"text/plain".
 *
 * @return the content type.
 */
String getContentType();

I'd like to clarify if returning null should be legal/allowed for 
implementation in general.

IOW, I'd like to say either:
- Call sites should be expected to handle null, or
- Implementations MUST not return null

WDYT?
Gary

Re: Monitoring discards from async logging

[Matt Sicker]

We’ve got a new proposal from Piotr on this front now which sort of emulates 
his suggestion for 3.x but applied to 2.x (since that doesn’t have the DI 
system). I like the approach there as it provides enough of a hook for users to 
customize whatever metrics library they wish to enhance some components however 
they see fit (similar to Spring’s BeanPostProcessor functionality).

> On Apr 12, 2024, at 06:33, Piotr P. Karwasz  wrote:
> 
> Hi Volkan,
> 
> On Thu, 11 Apr 2024 at 21:26, Volkan Yazıcı  wrote:
>> *Slightly longer answer:* Log4j has never had a holistic observability view
>> to its internals. We need something new (if not, from scratch), and
>> preferably, applicable to not only async. logging, but all components
>> wherever this can be needed. This is time consuming hard work.
>> Piotr and I, sort of, the only active Log4j maintainers nowadays, are
>> swamped with other priorities. All that said, you can contract a maintainer
>>  to get this
>> rolling.
> 
> I think you meant "you can contribute the functionality or contract
> **any** developer to do it". ;-) The way you wrote it sounds horrible.
> 
> Adam, to be entirely clear:
> 
> * Volkan and I are **not** the only maintainers. If a majority of
> maintainers support your PR, personally I will not block it, although
> I prefer to keep implementation details encapsulated as much as
> possible, so we can refactor them at any time,
> 
> * We would like to have a holistic solution to instrumenting Log4j
> Core. Even though Volkan and I undeniably work a lot on Log4j as part
> of our job, adding metrics to Log4j is not covered by our clients. So
> we cannot afford to deliver it ourselves in our spare time,
> 
> * Volkan probably meant that a Log4j maintainer could deliver this
> work faster, but we really don't care who contributes it. If you can
> contribute such a PR, we would be happy to review it. However before
> starting such a major endeavour you should either discuss the details
> on this mailing list or you can contact us directly and we can
> organize a video call with the rest of the PMC,
> 
> * Let me stress that we treat all PRs equally, whether coming from a
> project founder, PMC member or external contributors. My
> co-maintainers could probably tell you more about it. ;-)
> 
> Piotr

Re: Better Javadoc/Impl for org.apache.logging.log4j.core.Layout.getContentType()

[Matt Sicker]

We’ve got nullability annotations now, so we should be defining them. For use 
cases like this, I’d examine how existing things work and then document the 
nullability as such. For new code, I prefer things to be non-null except for 
cases where it makes sense to use null rather than, say, passing Optional 
parameters to methods or things like that.

> On Apr 12, 2024, at 08:55, Gary D. Gregory  wrote:
> 
> Hi All,
> 
> Our current Javadoc says:
> 
>/**
> * Returns the content type output by this layout. The base class returns 
> "text/plain".
> *
> * @return the content type.
> */
>String getContentType();
> 
> I'd like to clarify if returning null should be legal/allowed for 
> implementation in general.
> 
> IOW, I'd like to say either:
> - Call sites should be expected to handle null, or
> - Implementations MUST not return null
> 
> WDYT?
> Gary

Re: Monitoring discards from async logging

[Thomas, Adam]

Matt,

That sounds appealing, but do you have to have this hook registered by the time 
the disruptor is set up? That's my fundamental concern with a lot of these 
interception strategies - if your library/framework is called (which ours is), 
instead of being the entry point (as many others often are), logging will most 
likely already be initialized by the time control is handed to you. By the time 
my code gets a chance to inspect the LoggerContext, log4j has already decided 
which policy to use, instantiated it, and installed it.

-Adam

On 4/12/24, 11:49 AM, "Matt Sicker" mailto:m...@musigma.org>> wrote:

We’ve got a new proposal from Piotr on this front now which sort of emulates 
his suggestion for 3.x but applied to 2.x (since that doesn’t have the DI 
system). I like the approach there as it provides enough of a hook for users to 
customize whatever metrics library they wish to enhance some components however 
they see fit (similar to Spring’s BeanPostProcessor functionality).

> On Apr 12, 2024, at 06:33, Piotr P. Karwasz  > wrote:
>
> Hi Volkan,
>
> On Thu, 11 Apr 2024 at 21:26, Volkan Yazıcı  > wrote:
>> *Slightly longer answer:* Log4j has never had a holistic observability view
>> to its internals. We need something new (if not, from scratch), and
>> preferably, applicable to not only async. logging, but all components
>> wherever this can be needed. This is time consuming hard work.
>> Piotr and I, sort of, the only active Log4j maintainers nowadays, are
>> swamped with other priorities. All that said, you can contract a maintainer
>>  
>> ;> to get 
>> this
>> rolling.
>
> I think you meant "you can contribute the functionality or contract
> **any** developer to do it". ;-) The way you wrote it sounds horrible.
>
> Adam, to be entirely clear:
>
> * Volkan and I are **not** the only maintainers. If a majority of
> maintainers support your PR, personally I will not block it, although
> I prefer to keep implementation details encapsulated as much as
> possible, so we can refactor them at any time,
>
> * We would like to have a holistic solution to instrumenting Log4j
> Core. Even though Volkan and I undeniably work a lot on Log4j as part
> of our job, adding metrics to Log4j is not covered by our clients. So
> we cannot afford to deliver it ourselves in our spare time,
>
> * Volkan probably meant that a Log4j maintainer could deliver this
> work faster, but we really don't care who contributes it. If you can
> contribute such a PR, we would be happy to review it. However before
> starting such a major endeavour you should either discuss the details
> on this mailing list or you can contact us directly and we can
> organize a video call with the rest of the PMC,
>
> * Let me stress that we treat all PRs equally, whether coming from a
> project founder, PMC member or external contributors. My
> co-maintainers could probably tell you more about it. ;-)
>
> Piotr

Re: Monitoring discards from async logging

[Piotr P. Karwasz]

Hi Adam,

On Fri, 12 Apr 2024 at 21:42, Thomas, Adam  wrote:
> That sounds appealing, but do you have to have this hook registered by the 
> time the disruptor is set up? That's my fundamental concern with a lot of 
> these interception strategies - if your library/framework is called (which 
> ours is), instead of being the entry point (as many others often are), 
> logging will most likely already be initialized by the time control is handed 
> to you. By the time my code gets a chance to inspect the LoggerContext, log4j 
> has already decided which policy to use, instantiated it, and installed it.

My draft proposal[1] uses `ServiceLoader` to load all available
`InstrumentationService` implementations, so it happens before the
creation of the first `Logger` and the `AsyncLoggerDisruptor`.
We could obviously develop a system to allow late registrations of
`InstrumentationService`. Most of the components I expose in the PR
are tied to a `Configuration` so triggering a reconfiguration could
allow your code to instrument most of them.

Unfortunately the list does not include the `MessageFactory` used by
already created loggers or the disruptor attached to
`AsyncLoggerContext` (but it includes the disruptor used by
`AsyncLoggerConfig`s). You probably want to implement a "push" counter
for the discarded messages, so you get notified immediately whenever a
discard event occurs?

Feel free to comment on the PR or branch the code to correct it.

Piotr

[1] https://github.com/apache/logging-log4j2/pull/2469

Re: Monitoring discards from async logging

[Thomas, Adam]

This setup should work for us. While we don’t control logging initialization, 
we will reliably be present on the classpath at initialization time, and 
ServiceLoader should have no problems finding our implementation.

> You probably want to implement a "push" counter for the discarded messages, 
> so you get notified immediately whenever a discard event occurs?

For my use-case, no, periodic reporting is fine, but I'm happy to do the 
batching on my end.

-Adam

On 4/12/24, 2:50 PM, "Piotr P. Karwasz" mailto:piotr.karw...@gmail.com>> wrote:

Hi Adam,


On Fri, 12 Apr 2024 at 21:42, Thomas, Adam mailto:adamt...@amazon.com.inva>lid> wrote:
> That sounds appealing, but do you have to have this hook registered by the 
> time the disruptor is set up? That's my fundamental concern with a lot of 
> these interception strategies - if your library/framework is called (which 
> ours is), instead of being the entry point (as many others often are), 
> logging will most likely already be initialized by the time control is handed 
> to you. By the time my code gets a chance to inspect the LoggerContext, log4j 
> has already decided which policy to use, instantiated it, and installed it.


My draft proposal[1] uses `ServiceLoader` to load all available
`InstrumentationService` implementations, so it happens before the
creation of the first `Logger` and the `AsyncLoggerDisruptor`.
We could obviously develop a system to allow late registrations of
`InstrumentationService`. Most of the components I expose in the PR
are tied to a `Configuration` so triggering a reconfiguration could
allow your code to instrument most of them.


Unfortunately the list does not include the `MessageFactory` used by
already created loggers or the disruptor attached to
`AsyncLoggerContext` (but it includes the disruptor used by
`AsyncLoggerConfig`s). You probably want to implement a "push" counter
for the discarded messages, so you get notified immediately whenever a
discard event occurs?


Feel free to comment on the PR or branch the code to correct it.


Piotr


[1] https://github.com/apache/logging-log4j2/pull/2469