Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add service to attach metrics to Log4j Core #2469

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

ppkarwasz
Copy link
Contributor

This PR is an evolution of #1943, that adds an easy to use way to add metrics and other instrumentation to Log4j Core.

It introduces an InstrumentationService class that is used each time a component on the "logging path" is created or added to the Configuration. Most notably:

  • instrumentMessageFactory is called each time a logger is created,
  • each time a logger configuration is added instrumentLogEventFactory, instrumentReliabilityStrategy and instrumentLoggerConfig are called,
  • each time an asynchronous element is created (AsyncLoggerDisruptor, AsyncLoggerConfigDisruptor or AsyncAppender) the instrumentQueueFullPolicy` method is called,
  • the startup of a disruptor-based element additionally causes an instrumentRingBuffer call,
  • finally each time an appender is added to the configuration an instrumentAppender call happens.

Most of these methods can be use twofold:

  • they can return a wrapped component, so that "push" metrics can be implemented,
  • they can return the parameter of the method itself.

What do you think? cc/ @adamthom-amzn, @jvz

Looking at the currently existing solutions, micrometer-metrics (cf. Log4j2Metrics) could replace their filter-based solution to one based on instrumentLogEventFactory.

Which of the other method's could be useful? Are the additional parameters enough to define all the dimensions of the metrics?

Note: this PR is a draft for now, since it lacks documentation and tests.

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the 3.x approach will be via InstancePostProcessor or similar, this makes sense to me. I like how this doesn't require us to define any sort of metrics interfaces!

@adamthom-amzn
Copy link

At first glance, I quite like this approach, and it would help with some other instrumentation I've hacked in - I observe configuration changes to modify all attached appenders to add a filter that tracks appender throughput, and since I was already reflectively pulling out AsyncQueueFullPolicies, I was also grabbing RingBuffers to monitor them as well. Being able to decorate these components is appealing.

Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much liked the idea of making it possible to "intercept" component instantiation. This is a solid way to move forward with metrics without polluting the entire code base. Thanks so much for taking care of this @ppkarwasz! 💯 I would appreciate certain changes:

  1. I find it really difficult to buy the idea of #instrumentX() methods. This not only leaks internal details to a public API, it makes it a burden to grow the API: Every new component will/might need its #instrumentX() method. We should rather have a single entry point: Object instrumentInstance(Object). Implementations can selectively instrument instances they are interested in.
  2. I was expecting two changes: 1) augment plugin system to allow users to intercept the instantiation, and 2) use plugin system for every instantiation call site. Instead, I only see manually added interception points just to make @adamthom-amzn (in Expose disruptor discard count in AsyncLoggerContext #1927) happy. I want to believe that my understanding is wrong and there is something more to it I cannot see, hence I would appreciate some explanation here. Otherwise I doubt if this is acceptable.

Nit: IMHO, "instrumentation" means something else in programming jargon; what we are doing is rather [checking my Spring notes...] "interception". (I am fine with a "post-processor", as @jvz hinted, too.)

// TODO: Adapt after
// https://github.com/apache/logging-log4j2/issues/2379
// is fixed.
final MessageFactory actualMessageFactory = InstrumentationService.getInstance()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I agree with this approach. It is basically making InstrumentationService a factory for wrappers. I would rather see things that should be instrumented implement a well defined interface for providing the instrumented data such as https://github.com/apache/logging-flume/tree/trunk/flume-ng-core/src/main/java/org/apache/flume/instrumentation does. I think the temptation to do more than instrumentation is way too great with the current approach. It also means that InstrumentationServcie has to be aware of every type of thing that can be instrumented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vy I would NOT object to "intercepting" instantiation so long as a) it can't wrap the object being returned and b) it can only modify the created object if that object permits it. In my mind instrumentation means the ability to capture statistics or perform an allowed operation, not completely replace the thing being instrumented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants