-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update instrumentation with more call sites and instrumentation clearing endpoint #698
Conversation
} | ||
|
||
public Set<String> getAndClearUsedProperties() { | ||
synchronized (usedProperties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about double wrapping as in the other PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.