-
Notifications
You must be signed in to change notification settings - Fork 118
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
Disabling sdk feature #219
Conversation
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 some fields need to be populated in the segments even when disabled. Either that or you need to add the disabled check to more methods of segment
.
for anyone interested in testing this change, add this line to your go.mod: replace github.com/aws/aws-xray-sdk-go => github.com/bhautikpip/aws-xray-sdk-go Disable-SDK and then run:
|
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.
LGTM
xray/segment.go
Outdated
@@ -61,6 +62,13 @@ func BeginSegment(ctx context.Context, name string) (context.Context, *Segment) | |||
} | |||
|
|||
func BeginSegmentWithSampling(ctx context.Context, name string, r *http.Request, traceHeader *header.Header) (context.Context, *Segment) { | |||
// If SDK is disabled then return with an empty segment | |||
disableKey := os.Getenv("AWS_XRAY_SDK_DISABLED") |
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.
Does it make sense to put this in the Config in config.go
? If not, at least we should extract a function for this instead of copying it everywhere.
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.
That's a fair point. We can extract a function instead of writing this logic everywhere. Also, as per my understanding even if we put this in config.go we still have to put this logic in the entry-point of the publicly exposed methods.
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.
Some nits and golint otherwise LGTM
xray/segment.go
Outdated
@@ -61,6 +62,13 @@ func BeginSegment(ctx context.Context, name string) (context.Context, *Segment) | |||
} | |||
|
|||
func BeginSegmentWithSampling(ctx context.Context, name string, r *http.Request, traceHeader *header.Header) (context.Context, *Segment) { | |||
// If SDK is disabled then return with an empty segment | |||
sdkDisable := Disabled() |
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.
Is there a reason to have this local variable instead of just if Disabled()
?
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.
Makse sense! will modify it
xray/segment.go
Outdated
@@ -271,8 +286,23 @@ func NewSegmentFromHeader(ctx context.Context, name string, r *http.Request, h * | |||
return con, seg | |||
} | |||
|
|||
// Check if SDK is disabled | |||
func Disabled() bool { |
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.
Especially if removing the local variable, probably good to name this func SdkDisabled()
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.
right.
Issue #, if available:
Description of changes:
Added environment variable support to disable SDK feature.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.