Create a new SupportabilityMetrics class rather than using alpha Metrics #2353
Conversation
jkwatson
requested review from
anuraaga,
iNikem,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| private final boolean agentDebugEnabled; | ||
| private final Consumer<String> reporter; | ||
|
|
||
| private final ConcurrentMap<String, EnumMap<SpanKind, AtomicInteger>> suppressionCounters = |
There was a problem hiding this comment.
Let's use LongAdder instead of atomic integer
| return; | ||
| } | ||
| // note: there's definitely a race here, but since this is just debug information, I think | ||
| // we can live with the possibility that we might lose a count or two. |
There was a problem hiding this comment.
Is the race because of the usage of enummap? Looks like it's easy enough for us to define a class with a final field per kind to avoid the worry.
There was a problem hiding this comment.
No, due to the two phase lookup.
There was a problem hiding this comment.
Since they're both compute if absent, it seems like it would be ok if the second map was also a concurrent map. But we can use a class instead.
There was a problem hiding this comment.
we could also just create a single layer with a String key that concatenates the bits.
There was a problem hiding this comment.
I'll play around with a class to hold a longadder for each kind, although that feels a bit wasteful since a given instrumentation almost always only creates a single kind of span.
There was a problem hiding this comment.
By the light of day, after some more thinking, I think you're right. The lack of concurrent safety on the EnumMap makes it worse than a race... the values might not even be visible across threads.
So, some possible solutions are:
- a compound key class (or string concat) on a single concurrent map
- a thread-safe class as the value of a single level map
- 2 levels of concurrent map with LongAdders as the leaf
Since this is only doing anything if you turn on agent debug, it probably doesn't matter all that much which we choose, although it would be great if we could have an efficient solution that could be always on, even in production (maybe it only dumps out the metrics on demand or something, rather than every n seconds).
There was a problem hiding this comment.
I've been doing some research, mostly about LongAdder. It looks like all of the heap memory used by a LongAdder is completely lazily initialized. So, having a few extra LongAdders around that are unused is actually very cheap. I'm thinking that choosing the 2nd option is probably the simplest and most efficient, both from a correctness and a memory perspective.
There was a problem hiding this comment.
I think you could also use an EnumMap<SpanKind, ConcurrentHashMap<String, LongAdder>> prepopulated with empty maps for all 5 enum values - this way the enum map would effectively be read-only and could be set in the constructor.
But to be honest I prefer your option 2 😄
There was a problem hiding this comment.
done. option 2 has been implemented.
|
|
||
| public SupportabilityMetrics start() { | ||
| if (agentDebugEnabled) { | ||
| Executors.newScheduledThreadPool(1).scheduleAtFixedRate(this::report, 5, 5, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Shouldn't we use a daemon thread for things like that? There's a DaemonThreadFactory in javaagent-tooling, with some minor changes it could be moved to instrumentation-api and used here.
There was a problem hiding this comment.
easy enough to just inline one that sets daemon here. will do.
| return; | ||
| } | ||
| // note: there's definitely a race here, but since this is just debug information, I think | ||
| // we can live with the possibility that we might lose a count or two. |
There was a problem hiding this comment.
I think you could also use an EnumMap<SpanKind, ConcurrentHashMap<String, LongAdder>> prepopulated with empty maps for all 5 enum values - this way the enum map would effectively be read-only and could be set in the constructor.
But to be honest I prefer your option 2 😄
jkwatson
force-pushed
the
supportability
branch
from
d9f20ec
to
92deaf9
Compare
No description provided.