Rebind log4j2 metrics if configuration is changed #5810
Conversation
* Use `PropertyChangeListener` to add the `MetricsFilter` to log4j2 if configuration is reloaded * When adding filters, check if the filter is already added * Keep `metricFilters` in a `HashMap` to avoid creating new filters each time configuration is reloaded Signed-off-by: Patrik Ivarsson <patrik.ivarsson@avanza.se>
Signed-off-by: Patrik Ivarsson <patrik.ivarsson@avanza.se>
Thanks for the review! That is a good point! I don't think it would be any big issue, as the old configuration is already stopped here, but I do think it would be cleaner to clean up any old references to I made a new commit here where I also do a cleanup of the old configuration instance. I also changed the changeListener to only do an update if the configuration instance is not the same object as the old instance (as no rebinding or cleanup should be needed then). |
pativa
force-pushed
the
log4j2-reconfigure-rebind
branch
from
96dafd3 to
4361d51
Compare
Signed-off-by: Patrik Ivarsson <patrik.ivarsson@avanza.se>
pativa
force-pushed
the
log4j2-reconfigure-rebind
branch
from
4361d51 to
41d0e7e
Compare
Intended to verify that we don't continually create more filters when reloading config Signed-off-by: Patrik Ivarsson <patrik.ivarsson@avanza.se>
| @@ -61,7 +64,9 @@ public class Log4j2Metrics implements MeterBinder, AutoCloseable { | |||
|
|
|||
| private final LoggerContext loggerContext; | |||
|
|
|||
| private final List<MetricsFilter> metricsFilters = new ArrayList<>(); | |||
| private final Map<String, MetricsFilter> metricsFilters = new HashMap<>(); | |||
There was a problem hiding this comment.
I don't want to block progress on this pull request, but in reviewing it, I've noticed something I think should be fixed perhaps first. We used to just have a single MetricsFilter instance. That was changed in #2183 when MetricsFilter was changed to take a LoggerConfig. That was subsequently changed in 66f59e1 when the reason it was needed was fixed in log4j itself. I think we should have at that time changed back to using a single instance of MetricsFilter since its input is always the same for a given instance of Log4j2Metrics - the MeterRegistry and tags. We're needlessly making multiple instances of equivalent MetricsFilters. Let me know if I'm missing something. I'll prepare a change for maintenance branches to stop making extraneous instances of MetricsFilter. If we get that merged, you can rebase your pull request on top of it and it will hopefully simplify things a bit.
/cc @jonatan-ivanov
There was a problem hiding this comment.
The case where it wouldn't always be the same MeterRegistry is if the same instance of Log4j2Metrics is bound to different registries. This came up before recently on whether that's expected/should be supported in all cases. I'm not sure we came to a conclusion as it depends on the implementation of the MeterBinder. We'll have to discuss this.
There was a problem hiding this comment.
I was wondering about why multiple MetricsFilter were needed, but as it was that way I left that implementation. But it would be considerably easier to handle this without having multiple MetricsFilter, yes (no reason to store it in a map and keeping track of which filter was used for which logger).
If you change it to having a single filter, I can rebase this branch onto that and simplify this implementation a bit.
Having a single MetricsFilter wouldn't support binding the same Log4j2Metrics to different registries though, as you say.
Describe the issue
This PR is intended to fix #5756
When log4j2 metrics are reconfigured, metrics will no longer be recorded. For example, if
monitorInterval(docs) is configured to 30 is used and configuration is reloaded after application start, metrics would only be recorded for the first 30 seconds.By using a
PropertyChangeListener, it looks like we can register theMetricsFilterin much the same way, but it will be rebound after reloading the configuration. All the previous tests seem to pass too when using this method of configuration.