-
Notifications
You must be signed in to change notification settings - Fork 435
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
contrib/aws/aws-sdk-go: open only a single span for retryable operations #777
Conversation
This can be used to verify that all opened spans have in fact been closed.
Adds a failing test that demonstrates that the aws integration opens multiple spans when an operation needs to perform retries and does not close them.
This change ensures that only a single span is opened for an AWS operation regardless of how many times a the underlying HTTP call is retried Fixes DataDog#769
@marcind Thanks for this. I think it might be valuable to include information on retries in the traces. If we don't want to give each send attempt its own span, we should at least tag the one span with the number of retries. |
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.
Hi, @marcind
We should include information on the number of retries in the traces. What do you think?
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.
Thanks! This is much better in my opinion. One last thing and we're done 🙂
assert.Len(t, mt.OpenSpans(), 1) | ||
child.Finish() | ||
assert.Empty(t, mt.OpenSpans()) |
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.
Question for you @gbbr , should the parent span get included in openSpans
? Currently it will not, which causes an asymmetry with finishedSpans
, since it does get added to that collection once it is closed.
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 don't understand the question. Any started span that is unfinished should be considered open. Perhaps you can explain in more detail?
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 test illustrates the problem. parent
and child
get created, but only child
goes through mocktracer
's StartSpan()
method. At that point, FinishedSpans()
is empty, and OpenSpans()
only contains child
. Once both spans get finished, FinishedSpans()
contains both spans. At the beginning, len(OpenSpans()) == 1
. At the end, len(FinishedSpans()) == 2
.
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.
Perhaps the answer here is that parent
should also be initialized via mt.StartSpan()
, instead of calling newSpan
directly. Thoughts?
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.
Perhaps the answer here is that parent should also be initialized via mt.StartSpan(), instead of calling newSpan directly.
Exactly!
assert.Len(t, mt.OpenSpans(), 1) | ||
child.Finish() | ||
assert.Empty(t, mt.OpenSpans()) |
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 don't understand the question. Any started span that is unfinished should be considered open. Perhaps you can explain in more detail?
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>
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.
Added some more thoughts. Thanks for bearing with me. I'll be on vacation soon, so I'm hoping @knusbaum will find some time to help you with the review and getting it in...
assert.Len(t, mt.OpenSpans(), 1) | ||
child.Finish() | ||
assert.Empty(t, mt.OpenSpans()) |
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.
Perhaps the answer here is that parent should also be initialized via mt.StartSpan(), instead of calling newSpan directly.
Exactly!
This change ensures that only a single span is opened for an AWS operation regardless of how many times the underlying HTTP call is retried. This is achieved by only opening a span on the first attempt and doing nothing on any subsequent retry attempts.
To facilitate testing this change a new method is added to mocktracer which allows any unfinished spans to be inspected.
Fixes #769