Remove thread.name from metrics
#14061
Conversation
The committers listed above are authorized under a signed CLA. |
| // Use an access-ordered LinkedHashMap so we get a bounded LRU cache | ||
| private final Map<String, Consumer<RecordedEvent>> perThread = | ||
| new LinkedHashMap<String, Consumer<RecordedEvent>>(16, 0.75F, true) { | ||
| @Override | ||
| protected boolean removeEldestEntry(Map.Entry<String, Consumer<RecordedEvent>> eldest) { | ||
| // Bound this map to prevent memory leaks with fast-cycling thread frameworks | ||
| return size() > 512; | ||
| } | ||
| }; |
There was a problem hiding this comment.
is this map needed now that the thread name isn't being on the metrics?
There was a problem hiding this comment.
Certainly not for correctness reasons. I didn't see any explicit documentation around this map, but after reading through the code my impression was this was mostly a performance optimization to reduce allocations of Consumer<RecordedEvent> instances.
Since this was previously using an unsynchronized hashmap, it does appear to me that the invocation of these consumers is all single-threaded (I haven't worked directly with JFR before, so maybe that's not true?); it smells to me like there's no contention or throughput reasons to have this cache other than to reduce allocations.
There was a problem hiding this comment.
If that jives with your understanding, I could simply remove the cache. While all the little allocations of the PerThread*Handler inner classes probably aren't a problem for the use-cases I'm coming from (high-scale ecommerce), I imagine there are definitely existing OTEL use-cases where it would be, especially on older and/or smaller JVMs.
With that in mind, I'd actually prefer to jump all the way to inlining the PerThread*Handler-based logic directly into the AbstractThreadDispatchingHandler-subclasses.
There was a problem hiding this comment.
With that in mind, I'd actually prefer to jump all the way to inlining the PerThread*Handler-based logic directly into the AbstractThreadDispatchingHandler-subclasses.
I've gone ahead and done this, should be easy to revert if needed.
|
|
||
| // FIXME doesn't actually do any grouping, but should be safe for now | ||
| // FIXME only handles substrings of contiguous digits -> a single `x`, but should be good | ||
| // enough for now | ||
| @Nullable | ||
| public String groupedName(RecordedEvent ev) { |
There was a problem hiding this comment.
is this used anywhere now that AbstractThreadDispatchingHandler was deleted?
|
@jhayes2-chwy you need to sign the CLA in order to get the PR merged |
Indeed; I've been working with my company to determine if we have a Corporate CLA, so I'm still waiting on that. |
As outlined in #13407 and #14047, the
thread.nameattribute can create very high cardinality in many cases, and also contributes to a memory leak in the collection mechanism of those metrics. This PR is aimed at fixing both issues.The affected metrics are:
jvm.memory.allocationjvm.cpu.longlockjvm.network.iojvm.network.time