-
Notifications
You must be signed in to change notification settings - Fork 98
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
improved ForcedSampling support #232
Conversation
return segment; | ||
} else if (this.getSamplingStrategy().isForcedSamplingSupported()) { | ||
Segment segment = beginSegment(name); | ||
segment.setSampled(false); |
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.
From what I understand of ForcedSampling is that it can be used to force sample a segment which is un-sampled when created. Or is it only to keep the segment in context even though it is un-sampled?
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.
Forced sampling forces the creation of a regular instead of no-op segment when not sampled, which also places it in context. Then forceSamplingOfCurrentSegment
can be used in conjunction to actually switch an unsampled segment to sampled.
@@ -149,7 +155,6 @@ private synchronized void startPoller() { | |||
@Override | |||
public boolean isForcedSamplingSupported() { | |||
//TODO address this |
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.
This is addressed now right? :)
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.
Fair enough haha
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.
I think the constructor parameters are OK - the builder seems nice if we were to also hide the classes, but that would be breaking, so maybe a future cleanup
Issue #, if available:
#230
Description of changes:
Adds check for
ForcedSamplingSupport
tobeginSegmentWithSampling
. Also makesForcedSampling
a configurable property for the existingCentralized
andLocalized
sampling strategies via new constructors. I did not add them to theAll
orNo
strategies since forced sampling doesn't make sense in either case, they should be all or nothing, respectively.I used constructor parameters to keep the new field
final
. One alternative would be to implement builder classes for the strategies (4 new builder classes seems overkill tho) or just one sampling strategy builder that works like:Thoughts @anuraaga ?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.