Ignore noop children when polling composite meters #4093
Conversation
When a CompositeMeterRegistry is used and one of its CompositeMeters is polled (its value is fetched), the returned value can depend on the order of the registries inside of the composite if the composite contains a registry that has any NoopMeters. Example: a CompositeMeterRegistry contains two registries, A and B. We create a counter in the composite and increment it once. After this both A and B contain one counter but lets say that in A the counter is ignored so it will be noop. When the count called on CompositeCounter, it can return either 0 (if NoopCounter was used) or 1 (if the non-noop counter was used). In order to fix this, we can ignore the NoopMeters when Meters are polled in a composite registry. Closes micrometer-metricsgh-1441
| final Iterator<T> i = children.values().iterator(); | ||
| if (i.hasNext()) | ||
| return i.next(); | ||
| for (T next : children.values()) { |
There was a problem hiding this comment.
This should not be on the "hot path" (i.e.: recording a measurement or retrieving meters) but it will be called every time a value is fetched for publishing.
I'm thinking if there might be another way to implement this: modifying the children collection so that it has some priority ordering so the non-noop meters in it take precedence and will be "in front" of the collection (if any). That solution though might be more problematic since it seems a bit more complicated and it can be on a "hotter" path (meter creation).
There was a problem hiding this comment.
Have you seen this PR? The approach is completely different but certainly more efficient. We should understand if there is any hidden side effect if the meter is not added..
| if (!(next instanceof NoopMeter)) { | ||
| return next; | ||
| } | ||
| } | ||
|
|
||
| // There are no child meters. Return a lazily instantiated no-op meter. | ||
| final T noopMeter = this.noopMeter; |
There was a problem hiding this comment.
If we are at this point, the children collection was either empty or contains only noop meters. In the second case we could return it the first value (like we did before this change) but I'm not sure it worth the hassle.
|
You are merging this into |
Because I think we should consider this as a bug not a new feature. |
|
Hi, |
|
Hi @jonatan-ivanov, I've implemented a solution on top of your PR that addresses the "hot path" issue, please have a look. Feel free to incorporate the changes here, or if you want I can open a separate PR. Also, there are two alternative solutions I have considered. If any of those sound better to you, please let me know:
|
Szia, No news, we need to review and discuss it. Adding 👍🏼 to the issue could help with prioritizing.
Hi, I'm not sure I understand, My comment about the "hot path" is saying that what I'm doing in the PR should not be on the hot path so it should not have a hot path issue. That part of the code should only called once per every publishing but depending on the setup add/remove registries could be even less frequent, we should definitely consider this option too. |
|
I'm afraid I misunderstood your comment then, sorry for that... Should there be anything I can do to help, please let me know. |
|
Can we merge this? |
When a CompositeMeterRegistry is used and one of its CompositeMeters is polled (its value is fetched), the returned value can depend on the order of the registries inside of the composite if the composite contains a registry that has any NoopMeters.
Example: a CompositeMeterRegistry contains two registries, A and B. We create a counter in the composite and increment it once. After this both A and B contain one counter but lets say that in A the counter is ignored so it will be noop.
When the count called on CompositeCounter, it can return either 0 (if NoopCounter was used)
or 1 (if the non-noop counter was used).
In order to fix this, we can ignore the NoopMeters when Meters are polled in a composite registry.
Closes gh-1441