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

logback instrumentation module for 1.X #1091

Open
lesiak opened this issue Oct 4, 2024 · 9 comments
Open

logback instrumentation module for 1.X #1091

lesiak opened this issue Oct 4, 2024 · 9 comments
Assignees

Comments

@lesiak
Copy link

lesiak commented Oct 4, 2024

In Prometheus client 1.X, an equivalent for simpleclient_logback is missing.

simpleclient_logback contained only one class InstrumentedAppender.
It is easy to come up with an equivalent for 1.X (code below) but it would be great to have official support for both logback and log4j2.

I think a prominent use case is a high number of error logs from a given logger in a given time window.

Code for 1.X

package io.prometheus.metrics.instrumentation.logback;

import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.UnsynchronizedAppenderBase;
import io.prometheus.metrics.core.metrics.Counter;

public class InstrumentedAppender extends UnsynchronizedAppenderBase<ILoggingEvent> {
    public static final String METER_NAME = "logback_appender_total";

    private static final Counter defaultCounter = Counter.builder().name(METER_NAME)
            .help("Logback log statements at various log levels")
            .labelNames("level")
            .register();

    private final Counter counter = defaultCounter;


    @Override
    protected void append(ILoggingEvent event) {
        switch (event.getLevel().toInt()) {
            case Level.TRACE_INT:
                this.counter.labelValues("level", "trace").inc();
                break;
            case Level.DEBUG_INT:
                this.counter.labelValues("level", "debug").inc();
                break;
            case Level.INFO_INT:
                this.counter.labelValues("level", "info").inc();
                break;
            case Level.WARN_INT:
                this.counter.labelValues("level", "warn").inc();
                break;
            case Level.ERROR_INT:
                this.counter.labelValues("level", "error").inc();
                break;
            default:
                break;
        }
    }
}

As 1.X is a good opportunity to redesign, is this one-to-one approach a good one and can be submitted as an MR?
I've seen an alternative in form of TurboFilter in Micrometer LogbackMetrics but appender approach seems more straightforward to me.

@zeitlinger
Copy link
Member

yes - it's a good opportunity for a redesign and a pr would be welcome

@Tharanishwaran
Copy link

can i work on this issue ?

@zeitlinger
Copy link
Member

yes, that would be great

@Tharanishwaran
Copy link

Tharanishwaran commented Oct 10, 2024

Hi @zeitlinger can you assign me to this issue ?

@Tharanishwaran
Copy link

@zeitlinger Thanks for assigning me! I'll start working on it and update you soon

@lesiak
Copy link
Author

lesiak commented Oct 10, 2024

@Tharanishwaran if you would like to take the code from my original post as a starting point, I think the questions that need some attention are:

  • is a Turbo filter superior in any aspect to an appender? From my point of view appender fits better semantically (we are processing lugging events in a data sink) and functionally - we can add filters to an appender.
  • performance comparison between the approaches would be great
  • The code in original post uses UnsynchronizedAppenderBase<ILoggingEvent> as a base class. It has some internal synchronization, which is likely unnecessary - Counter class is thread-safe. Again, this will gain us a few performance points.
  • Currently the appender would need to be added in Logback config. Maybe it would be more consistent if we could register it in Prometheus registry via register method.

@Tharanishwaran
Copy link

@lesiak Thank you for your detailed suggestions. They're extremely helpful as I start working on this issue. Here's my plan:

  • I'll implement both the appender and Turbo filter approaches for comparison.
  • I'll create benchmarks to compare their performance.
  • I'll look into optimizing synchronization in the base class.
  • I'll explore implementing registration via the Prometheus registry method.

Additionally, I'll consider handling custom log levels and potential configuration options.
Do you have any thoughts on which configuration options might be most valuable to users?

I'll provide updates as I progress. Thank you again for your guidance

@lesiak
Copy link
Author

lesiak commented Oct 11, 2024

I don't see a lot of configuration options.
If you go with MeterBinder approach to implement registration with Prometheus registry, then some options available in Logback config could be helpful - like adding a custom filter.

@Tharanishwaran
Copy link

@lesiak Thanks for the tips! I'll check out the MeterBinder approach and custom Logback filters. I'll keep it simple otherwise. I'll let you know how it goes as I work on it.

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

No branches or pull requests

3 participants