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 does not work with multiple registries and non-root loggers #5893

Closed
shakuzen opened this issue Feb 5, 2025 · 0 comments
Closed
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
Milestone

Comments

@shakuzen
Copy link
Member

shakuzen commented Feb 5, 2025

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).

Originally posted by @pativa in #5818 (comment)

@shakuzen shakuzen added bug A general bug module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component labels Feb 5, 2025
@shakuzen shakuzen added this to the 1.13.11 milestone Feb 5, 2025
@shakuzen shakuzen changed the title Log4j2Metrics does not work with multiple registries a non-root loggers Log4j2Metrics does not work with multiple registries and non-root loggers Feb 5, 2025
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
Projects
None yet
Development

No branches or pull requests

1 participant