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

Log4j2Metrics creates more MetricsFilter instances than needed #5818

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

shakuzen
Copy link
Member

@shakuzen shakuzen commented Jan 23, 2025

Before, we were needlessly making multiple instances of MetricsFilter that were functionally equivalent. This reduces the MetricsFilter instances to one per registry to which a Log4j2Metrics instance is bound. This will reduce allocations and memory usage slightly.

@shakuzen shakuzen added bug A general bug performance Issues related to general performance module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component labels Jan 23, 2025
@shakuzen shakuzen added this to the 1.13.11 milestone Jan 23, 2025
@shakuzen shakuzen changed the title Log4j2Metrics creates more MetricsFilter instances than needed Log4j2Metrics creates more MetricsFilter instances than needed Jan 23, 2025
Before, we were needlessly making multiple instances of `MetricsFilter` that were functionally equivalent. This reduces the instances to one per registry to which Log4j2Metrics is bound. This will reduce allocations and memory usage slightly.
@shakuzen
Copy link
Member Author

Merging before review so we don't keep blocking #5810, but anyone feel free to leave review post merge and we can follow-up with anything that comes up.

@shakuzen shakuzen merged commit d645839 into micrometer-metrics:1.13.x Jan 23, 2025
7 checks passed
@shakuzen shakuzen deleted the log4j2-perf branch January 23, 2025 08:11
@pativa
Copy link
Contributor

pativa commented Jan 29, 2025

@shakuzen
While reading the code while looking at #5810 I saw another issue in this fix (or rather, this does not fully support binding to multiple MeterRegistry). The issue is that it only works on rootLoggerConfig, for all other logger configs the second filter will not be added.

Here is a modified test from the test added in this PR that does not pass as the non-root logger configuration is not tracked for the second MeterRegistry:

    @Test
    void multipleRegistriesCanBeBound() {
        LoggerContext loggerContext = new LoggerContext("test");

        LoggerConfig loggerConfig = new LoggerConfig("com.test", Level.INFO, false);
        Configuration configuration = loggerContext.getConfiguration();
        configuration.getRootLogger().setLevel(Level.INFO);
        configuration.addLogger("com.test", loggerConfig);
        loggerContext.updateLoggers(configuration);

        MeterRegistry registry2 = new SimpleMeterRegistry();

        Log4j2Metrics log4j2Metrics = new Log4j2Metrics(emptyList(), loggerContext);
        log4j2Metrics.bindTo(registry);

        // verify root logger
        Logger logger = loggerContext.getLogger(Log4j2MetricsTest.class);
        logger.info("Hello, world!");
        assertThat(registry.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(1);

        // verify other logger
        Logger logger2 = loggerContext.getLogger("com.test");
        logger2.info("Using other logger than root logger");
        assertThat(registry.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(2);

        log4j2Metrics.bindTo(registry2);

        logger.info("Hello, world!");
        assertThat(registry.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(3);
        assertThat(registry2.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(1);

        logger2.info("Using other logger than root logger");
        assertThat(registry.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(4);
        // this final check does not pass as the log event is not properly counted
        assertThat(registry2.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(2);
    }

The issue is the instanceof checks in Log4j2Metrics, it needs to check the correct instance of MetricsFilter to properly support registering multiple MeterRegistry (which is a very odd case anyway).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module performance Issues related to general performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants