Solve the duplicate metrics definition issue in Micrometer shim #4457
Conversation
mateuszrzeszutek
requested review from
anuraaga,
jack-berg,
jkwatson and
jsuereth
as code owners
Codecov Report
@@ Coverage Diff @@
## main #4457 +/- ##
=========================================
Coverage 90.02% 90.03%
- Complexity 4970 4972 +2
=========================================
Files 570 570
Lines 15384 15387 +3
Branches 1472 1472
=========================================
+ Hits 13850 13853 +3
Misses 1064 1064
Partials 470 470
Continue to review full report at Codecov.
|
|
|
||
| final class Bridging { | ||
|
|
||
| private static final ConcurrentMap<String, String> descriptionsCache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
I have a tiny worry that someone will be creating dynamically named meters and making this grow without bound, but I think that's a small worry that probably means they're doing sometime wrong with micrometer.
There was a problem hiding this comment.
If someone does that other areas of the metrics SDK will cause memory pressure at a faster pace than this.
|
|
||
| final class Bridging { | ||
|
|
||
| private static final ConcurrentMap<String, String> descriptionsCache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
If someone does that other areas of the metrics SDK will cause memory pressure at a faster pace than this.
|
@jack-berg @jkwatson @mateuszrzeszutek |
|
IMO this doesn't meet the criteria for a patch release since its not a regression, memory leak or deadlock. The change improves the ergonomics of bridging micrometer into opentelemetry, but nothing is specifically broken at the moment. |
Resolves #4381
I added a descriptions cache, only the first description seen will be used, the rest will just get ignored (pretty much the same behavior as
PrometheusMeterRegistry). I tried to use theMeterFilterfor that at first, but it turned outMeter.Iddoes not have any method for overriding the description.