Metric endpoint is summing up same metric #14497
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| } | ||
| else { | ||
| meters.addAll(registry.find(name).tags(tags).meters()); | ||
| Collection<Meter> meterFound = registry.find(name).tags(tags).meters(); | ||
| if (meterFound != null && meterFound.size() > 0) { |
There was a problem hiding this comment.
Micrometer guarantees that meterFound is non-null. Could use isEmpty() on second condition.
| @@ -112,15 +112,20 @@ private Tag parseTag(String tag) { | |||
| return Tag.of(parts[0], parts[1]); | |||
| } | |||
|
|
|||
| private void collectMeters(List<Meter> meters, MeterRegistry registry, String name, | |||
| Iterable<Tag> tags) { | |||
| private boolean findFirstMatchingMeters(List<Meter> meters, MeterRegistry registry, | |||
There was a problem hiding this comment.
Rather than modifying a parameter, this method could simply return a Collection.
|
@jkschneider , please review. |
| meters.addAll(registry.find(name).tags(tags).meters()); | ||
| Collection<Meter> metersFound = registry.find(name).tags(tags).meters(); | ||
| if (!metersFound.isEmpty()) { | ||
| return new ArrayList<>(metersFound); |
There was a problem hiding this comment.
No need to wrap this. The list returned is already an immutable copy.
There was a problem hiding this comment.
Thanks @jkschneider , the reason I am wrapping it into List is to avoid returning Collection from this method.
Collection<Meter> metersFound = registry.find(name).tags(tags).meters();
If we return Collection, the MetricEndpoint methods should be refactored so that Collection could be pass around instead of List interface OR the return type should be downcasted to List (which is dangerous as we dont have control on MeterRegistry#find)
Changing MetricEndpoint methods to pass Collection is minor refactoring, with following highlight.
will be change to
meters.iterator().next().getId();
Please confirm should I go ahead and do this minor change?
There was a problem hiding this comment.
The minor refactoring seems preferable to me to generating extra garbage in this case, as minor as that will be.
There was a problem hiding this comment.
Thanks @jkschneider, pls review.
philwebb
added
type: bug
philwebb
changed the title
philwebb
changed the title
|
@philwebb , @jkschneider , the issue is closed but PR is still open. Is there anything l can do to help you merge it? |
|
The issue has been closed because this PR supersedes it. All is good, we just need some time to review it. |
* pr/14497: Polish "Stop MetricsEndpoint from summing up same metrics" Stop MetricsEndpoint from summing up same metrics
|
@pulkitmehra Thanks for the fix. This is now in 2.0.x and master. |
This PR fixed issue #14066