Dummy flag to reduce operations on non sampled traces #194
Conversation
xray/segment.go
Outdated
| logger.Debugf("Beginning segment named %s", name) | ||
|
|
||
| cfg := GetRecorder(ctx) | ||
| dummySeg.assignConfiguration(cfg) |
There was a problem hiding this comment.
Is it necessary to pass the recorder in configuration for a DummySegment? The assignConfiguration method does a bunch of comparisons which should be avoided if we don't really need the configuration.
xray/segment.go
Outdated
| defer dummySeg.Unlock() | ||
|
|
||
| dummySeg.Name = name | ||
| dummySeg.TraceID = "dummy segment" |
There was a problem hiding this comment.
Can be an empty string or something more meaningful.
There was a problem hiding this comment.
+1 to here and for dummy subsegment, actually in Java we create a new trace ID. Probably best to not risk some weird validation errors and do the same here.
xray/segment.go
Outdated
| return context.WithValue(ctx, ContextKey, seg), seg | ||
| } | ||
|
|
||
| return BeginDummySegment(ctx, "Dummy Seg") |
There was a problem hiding this comment.
Better to use full name as "DummySegment"
xray/segment.go
Outdated
|
|
||
| dummySubSeg.ParentSegment = parent.ParentSegment | ||
| dummySubSeg.Name = name | ||
| dummySubSeg.TraceID = "dummy subsegment" |
xray/segment.go
Outdated
| return context.WithValue(ctx, ContextKey, seg), seg | ||
| } | ||
|
|
||
| return BeginDummySegment(ctx, "Dummy SubSeg") |
There was a problem hiding this comment.
Better to use full name as "DummySubsegment"
There was a problem hiding this comment.
The dummy segment here does not stub out methods that it should to achieve performance savings. This is because the dummies are still instances of real segments, and will thus invoke their real methods. Furthermore, since the dummy (sub)segments are not begun until the end of the conventional beginSegment/beginSubsegment functions, it actually takes longer to begin a dummy instance.
I think the design here could include making the segment struct in segment_model an interface instead. This would allow us to more closely follow the implementation in the Java SDK, where such interfaces made implementing dummy (sub)segments much easier.
There will be differences between the two SDKs because the dummy subsegment is never actually used in the Java SDK, but I think that could be an area the Go SDK improves upon.
xray/segment.go
Outdated
| defer dummySeg.Unlock() | ||
|
|
||
| dummySeg.Name = name | ||
| dummySeg.TraceID = "dummy segment" |
There was a problem hiding this comment.
+1 to here and for dummy subsegment, actually in Java we create a new trace ID. Probably best to not risk some weird validation errors and do the same here.
xray/segment.go
Outdated
| func BeginDummySegment(ctx context.Context, name string) (context.Context, *Segment) { | ||
| dummySeg := &Segment{parent: nil} | ||
| dummySeg.ParentSegment = dummySeg | ||
| logger.Debugf("Beginning segment named %s", name) |
There was a problem hiding this comment.
Nit: Should read Beginning dummy segment named %s
xray/segment.go
Outdated
| seg.Unlock() | ||
| } | ||
|
|
||
| // Begin DummySubSegment creates a in the case of no sampling to reduce memory footprint |
There was a problem hiding this comment.
Nit: Fix typo in comments
xray/segment.go
Outdated
| } | ||
|
|
||
| dummySubSeg := &Segment{parent: parent} | ||
| logger.Debugf("Beginning subsegment named %s", name) |
xray/segment.go
Outdated
| return context.WithValue(ctx, ContextKey, seg), seg | ||
| } | ||
|
|
||
| return BeginDummySegment(ctx, "Dummy SubSeg") |
xray/segment.go
Outdated
| return context.WithValue(ctx, ContextKey, seg), seg | ||
| } | ||
|
|
||
| return BeginDummySegment(ctx, "Dummy Seg") |
xray/segment.go
Outdated
| return context.WithValue(ctx, ContextKey, seg), seg | ||
| } | ||
|
|
||
| return BeginDummySegment(ctx, "DummySegment") |
There was a problem hiding this comment.
If the Segment is dummy, adding annotation and metadata should be no-op to save the memory footprint.
|
|
||
| // AddAnnotation allows adding an annotation to the segment. | ||
| func (seg *Segment) AddAnnotation(key string, value interface{}) error { | ||
| seg.Lock() |
There was a problem hiding this comment.
why do we lock and unlock before checking dummy?
There was a problem hiding this comment.
because we don't want any data race on segment struct. so it's a good idea to lock the struct before accessing it.
xray/segment.go
Outdated
| seg.InProgress = true | ||
|
|
||
| // check whether segment is dummy or not based on sampling decision | ||
| seg.isDummy() |
There was a problem hiding this comment.
can we fail faster by putting this immediately after seg.ParentSegment = parent.ParentSegment? We don't bother keeping the footprint in the parent segment as we won't send it out anyway.
There was a problem hiding this comment.
Yeah. we can put this right after seg.ParentSegment = parent.ParentSegment.
xray/segment.go
Outdated
| } | ||
|
|
||
| // check the segment is dummy segment or not | ||
| func (seg *Segment) isDummy() *Segment { |
There was a problem hiding this comment.
This method seems not necessary. It is only called at 2 places when begin a segment or subsegment. Customer doesn't need to call it as well. I'd suggest replacing the caller code with
if !seg.ParentSegment.Sampled
seg.Dummy = true
BTW The method name is confusing. isXYZ() should returns a boolean. Now the return type is never used (see the github warn below)
xray/segment.go
Outdated
| defer seg.Unlock() | ||
|
|
||
| // If segment is dummy we return | ||
| if seg.Dummy { |
There was a problem hiding this comment.
At creating segment we dealt with the segment children membership no matter dummy or not. If we return early here, would skipping the following code cause mem leak? i.e. it will always be "in progress" and the parent always keep the reference of it
bhautikpip
changed the title
Issue #, if available:
Description of changes:
Added support for creating DummySegment and DummySubSegments.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.