micrometer
micrometer copied to clipboard
micrometer-metrics
Add a meter filters changed callback to MeterRegistry
Please describe the feature request.
Add a onMeterFiltersChanged method, similar to onMeterRemoved and others, to the MeterRegistry.
This method would maintain a CopyOnWriteArrayList of listeners which would be invoked when users add a filter.
Rationale
When registerMeterIfNecessary is called, the meter id created by the meter builder is passed to getMappedId.
getMappedId invokes all the filters to compute the actual meter id.
But given meter ids are immutable, meter registration and lookup lead to the allocation of many Meter.Id instances.
If we had onMeterFiltersChanged, we could use the callback together with onMeterRemoved to maintain a cache of (name/tags) => meter. and save a lot of allocations.
Additional context In Vert.x we are (happy!) users of Micrometer as an implementation of the Vert.x Metrics SPI.
Because we dynamically get references to meters (one reason, among others, is that Vert.x users can provide custom tags for their servers, pools, ...etc), we make a lot of calls to registerMeterIfNecessary from hot paths.
If we could create a cache of (name/tags) => meter, we could save a lot of allocations: Meter.Id is the biggest part, but looking up meters also implies creating builders, config instances and a few others.
I can provide a PR if you don't have objections.
Also, I found that a lot of allocations were caused by meter filters which use Java streams and capturing lambdas. See https://github.com/vert-x3/vertx-micrometer-metrics/pull/158/commits/65facf2e5efbd4b03931fb7869dc6b227db4c26e
If you're interested I can also propose a PR to integrate this upstream, perhaps tracked by a separate issue?
Sorry for the gigantic delay... I'm adding an enhancement label for this and we'll discuss this internally ASAP
@marcingrzejszczak Was there any discussion on this?
It is also unclear if we should allow adding of meterfilters after then registry is initialized/created. The reason is that any meters that are created earlier may be invalid because a new meter-filter is added. Should the meter-filters be immutable?
@shakuzen / @jonatan-ivanov I believe adding meterfilters should be immutable or at the least should not be modified after a meter is registered.
Adding meterfilters to the registry once meters are created should be treated as an error state. This invalidates all the meters ever registered in the registry which means the registry is invalid. Do you guys agree on this?
Adding meterfilters to the registry once meters are created should be treated as an error state. This invalidates all the meters ever registered in the registry which means the registry is invalid. Do you guys agree on this?
Normal expected usage would be to configure all desired MeterFilters before registering any meters, yes. However, since we don't enforce that currently, and depending on the usage, users may not notice an issue even if they aren't following this. It depends what is registered before and whether a reference to the meter is kept or the builder is used each time. If the builder is used each time, they will end up with the meters having all filters applied, but they will also have meters registered without all the filters applied.
I think it would be good for us to understand the scenarios in which it is happening that MeterFilters are being configured late, and if something can be done to easily stop that and how we can best signal to users to not do that without breaking existing usage if avoidable. I think this is an area where there will often be different layers - a framework setting things up and providing instrumentation, and in addition, users' own configuration and instrumentation.
@tsegismont I'm terribly sorry we failed to give you feedback on this issue for so long. Is there a scenario vert.x users are encountering where MeterFilters are configured after meters are registered? How hard would it be to ensure MeterFilters are configured before any meters are registered? If I understood the request, it is ultimately to be able to optimize the retrieval of existing meters reliably. For that, I think it'd be best for us to try to solve it for all users rather than users needing to use a cache as you've described along with a meter filters changed callback. I think @lenin-jaganathan's proposal in #4856 could be one such way we get to there. But we need to figure out what to do about MeterFilters configured late (after a meter is registered). If that is the only purpose of the request for a meter filters changed callback, I think perhaps we can avoid exposing that as new API that users need to deal with if we can solve it internally.
@shakuzen Just to add to what @tsegismont added initially.
We also do the same sort of thing in our internal framework where we are forced to keep a cache to avoid going through filters again and again. We use spring-boot and hence we wire up all the meterfilters right after the registry is created before any meters are registered in it. (actuator does that for us)
And since adding meterfilter somewhere after this would mean an inconsistent meter registry we are ignoring this case and handling only when a meter is explicitly removed to house keep our cache.
Hi everyone, sorry I'm late to the party :blush:
I think it would be good for us to understand the scenarios in which it is happening that MeterFilters are being configured late
@shakuzen I'm sorry I can't help with that.
I filed this issue as I was looking for ways to improve performance for Vert.x Micrometer users. I wrote an article recently (not published yet) that explains our motivation and findings.
One of the changes we've made in Vert.x Micrometer is to use a meter cache for Vert.x event loops. That helps greatly for CPU bound workloads. The problem with using a cache is we need to assume that configuration of the registry is not modified after startup. In practice, that means not adding/removing meter filters.
As far as I'm concerned, I see no reason for changing filters after startup. If a user does this, they need to be sure that the affected meters are always used dynamically (i.e. looked up in the registry before usage).
If I understood the request, it is ultimately to be able to optimize the retrieval of existing meters reliably. For that, I think it'd be best for us to try to solve it for all users rather than users needing to use a cache as you've described along with a meter filters changed callback. I think @lenin-jaganathan's proposal in #4856 could be one such way we get to there. But we need to figure out what to do about MeterFilters configured late (after a meter is registered). If that is the only purpose of the request for a meter filters changed callback, I think perhaps we can avoid exposing that as new API that users need to deal with if we can solve it internally.
I'll take a look at the proposal in #4856 and will comment there.
If it wasn't clear already, yes, the only purpose of this issue was to cache meters reliably.
I'll take a look at the proposal in #4856
It seems #4856 (and the PR #4857 ) has the same purpose has the change we did in Vert.x Micrometer: caching meters using a meter id built before applying filters as cache keys.
@tsegismont thank you for taking a look and the feedback. Sorry again this took so long for us to act on. If we're able to get https://github.com/micrometer-metrics/micrometer/pull/4857 merged, then I think we'll close this issue as effectively resolved by that until someone shares a need for the requested callback other than the cache we'll have internally at that point. Hopefully that will simplify some things for vert.x as well. We'd love to have more datapoints verifying the change helped significantly reduce overhead.
Closing as we don't see a need for this anymore in light of #4857 being merged. If someone still has a need for this, please explain that and we can consider reopening this.