Traced aws client should allow requests to proceed without Segments present #54
Comments
|
Hello @NZKoz , thank you for contacting us regarding your issue. Yes, we should check whether the segment is nil or not in |
|
@luluzhao that fixed one of the cases, but I'm guessing you need a similar fast-return from endSubsegment. Is the new crash, the fix for that is: diff --git a/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go b/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go
index b290a4a..3018b0c 100644
--- a/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go
+++ b/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go
@@ -42,6 +42,9 @@ func beginSubsegment(r *request.Request, name string) {
func endSubsegment(r *request.Request) {
seg := GetSegment(r.HTTPRequest.Context())
+ if seg == nil {
+ return
+ }
seg.Close(r.Error)
r.HTTPRequest = r.HTTPRequest.WithContext(context.WithValue(r.HTTPRequest.Context(), ContextKey, seg.parent))
}If I add another nil check to endSubsegment it falls down again in NewClientTrace diff --git a/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go b/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go
index ec98208..053c702 100644
--- a/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go
+++ b/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go
@@ -77,7 +77,11 @@ var xRayAfterBuildHandler = request.NamedHandler{
var xRayBeforeSignHandler = request.NamedHandler{
Name: "XRayBeforeSignHandler",
Fn: func(r *request.Request) {
- ctx, _ := BeginSubsegment(r.HTTPRequest.Context(), "attempt")
+ ctx, seg := BeginSubsegment(r.HTTPRequest.Context(), "attempt")
+
+ if seg == nil {
+ return
+ }
ct, _ := NewClientTrace(ctx)
r.HTTPRequest = r.HTTPRequest.WithContext(httptrace.WithClientTrace(ctx, ct.httpTrace))
With those two fixes applies it runs fine, feel free to use those diffs if they're helpful. |
|
Hi @NZKoz , thank you for your effort in deep diving this issue and providing the related fix. We'll get this fix out as soon as possible. |
|
Any update @luluzhao, we're running with a manually managed cherry-pick of aws.go and it's slightly error prone 😄 |
|
@NZKoz, sorry for the late reply. We just released version |
|
Similar to the AWS Client, I think it should be possible for |
Wherever possible we try to include XRay tracing in our common wrapper libraries so that we can gradually benefit from tracing as more and more of our services and consumers start preparing segments. This works great with the http client, however the AWS instrumentation really doesn't like being passed requests which don't have segments embedded.
Ideally the instrumentation would be sufficiently unobtrusive that it silently continued when there is no active segment.
We did investigate embedding our own segments manually elsewhere but had the same issue reported in #51
Specifically
endSubsegmentassumes there's a segment in flight:aws-xray-sdk-go/xray/aws.go
Lines 46 to 50 in ddbaf76
And some of the hooks don't handle it at the beginning of the requests:
aws-xray-sdk-go/xray/aws.go
Lines 52 to 62 in ddbaf76
By contrast, other parts of the codebase assume that missing segments can happen, and check for nil when working with segments.
aws-xray-sdk-go/xray/aws.go
Lines 178 to 180 in ddbaf76
The specific crash we get is:
The text was updated successfully, but these errors were encountered: