Update instrumentation with more call sites and instrumentation clearing endpoint #698
Conversation
akang31
changed the title
| } | ||
|
|
||
| public Set<String> getAndClearUsedProperties() { | ||
| synchronized (usedProperties) { |
There was a problem hiding this comment.
Synchronizing on the map won't keep it from being modified concurrently by other threads, so there still a race condition where threadA calls getUsedProperties, threadB records a new usage, threadA clears the map, and now threadB's usage is lost.
The only way to avoid that would be to synchronize all methods that record access, in addition to this one. Not sure that'd be worth the performance hit, but in that case you might as well change the map back to a regular HashMap, instead of paying the ConcurrentHashMap tax.
There was a problem hiding this comment.
Per offline discussion, updated this to use an AtomicReference instead
| } | ||
|
|
||
| public Set<String> getAndClearUsedProperties() { | ||
| Set<String> ret = usedPropertiesRef.getAndSet(ConcurrentHashMap.newKeySet()); | ||
| return Collections.unmodifiableSet(new HashSet<>(ret)); |
There was a problem hiding this comment.
Same comment about double wrapping as in the other PR.
There was a problem hiding this comment.
Removed the double wrap here to just directly return the unmodifiableSet
The instrumentation currently is missing a few cases that are addressed here:
We also add the ability to clear used instrumentation to avoid repeatedly sending the same instrumentation data over the course of an application's lifetime.