ARTEMIS-4655 report logging metrics #4830
Conversation
| if (metricsConfiguration.isLogging()) { | ||
| new Log4j2Metrics().bindTo(meterRegistry); | ||
| } |
There was a problem hiding this comment.
Given the broker as a whole is not really tied to Log4j2 (but rather SLF4J API), it actually feels a little weird that this is. Especially as the broker (or micrometer) has no [passed-on] dependency on Log4J2, so this wont work in some cases. The config doc should perhaps be clearer that it is Log4j2-specific, and perhaps note that such a dependency would need to be provided if enabling this while e.g embedding / not using the distribution.
There was a problem hiding this comment.
I updated the docs to clarify this.
There was a problem hiding this comment.
Not sure the new doc is clear enough / goes far enough. This thing actually depends on Log4J API and Log4J Core, but neither the broker (no dep at all) or micrometer core (optional dep) have a hard dep on them to bring them in...so for anyone e.g embedding, it seems likely to blow up at this point unless some other dep has actually brought in log4j-core. Our tests happen to do that and so wont show any issue.
There was a problem hiding this comment.
See the latest update.
jbertram
force-pushed
the
ARTEMIS-4655
branch
2 times, most recently
from
21a2740
to
1e5f31f
Compare
...s/src/test/java/org/apache/activemq/artemis/tests/integration/plugin/LoggingMetricsTest.java
Outdated
Show resolved
Hide resolved
| if (metricsConfiguration.isLogging()) { | ||
| new Log4j2Metrics().bindTo(meterRegistry); | ||
| } |
There was a problem hiding this comment.
Not sure the new doc is clear enough / goes far enough. This thing actually depends on Log4J API and Log4J Core, but neither the broker (no dep at all) or micrometer core (optional dep) have a hard dep on them to bring them in...so for anyone e.g embedding, it seems likely to blow up at this point unless some other dep has actually brought in log4j-core. Our tests happen to do that and so wont show any issue.
jbertram
force-pushed
the
ARTEMIS-4655
branch
3 times, most recently
from
61cb2d4
to
1269a58
Compare
|
I ran the full test suite job on this and it failed at the new test. I pulled and rebased your changes locally and tried the test in isolation, and it also failed in the same way. Seems that something is needing tweaked. |
It may be useful to configure alerts for ERROR or WARN events in the log which may go unnoticed otherwise.
|
The test was working originally. I'm not sure how I broke it, but it's working again now. |
It may be useful to configure alerts for ERROR or WARN events in the log which may go unnoticed otherwise.