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. |
| assert.Len(t, mt.OpenSpans(), 1) | ||
| child.Finish() | ||
| assert.Empty(t, mt.OpenSpans()) |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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>
| assert.Len(t, mt.OpenSpans(), 1) | ||
| child.Finish() | ||
| assert.Empty(t, mt.OpenSpans()) |
There was a problem hiding this comment.
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