Add workaround for broken span propagation in streaming requests #2992
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolesnikovae
approved these changes
Feb 9, 2024
|
Thank you so much for doing this ! |
simonswine
pushed a commit
that referenced
this pull request
Feb 15, 2024
* Add workaround for broken span propagation in streaming requests * Fix lint error * Run go mod tidy
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2923
Background:
The current instrumentation for the HTTP client is based on https://github.com/opentracing-contrib/go-stdlib. This works well enough for standard HTTP requests but does not support streaming. This instrumentation library "finishes" the opened trace spans only when the response body gets closed, which doesn't happen for streaming requests where the initial response has an empty body and is never read / closed. The result is below. Note the disconnected spans at the bottom and the empty "HTTP Client" spans:
Proper fix:
Switching to an instrumentation library that has proper support for streaming is the ideal way to address this. We will eventually switch to Open Telemetry which should offer a good path forward. This is a bigger change and in the interest of improving trace analysis I am proposing a workaround.
Workaround:
When we detect an empty response body we finish the span directly. This means that the span will report an incorrect time, but this is still an improvement over the current state where this span was completely removed which broke the span tree. In addition, I've merged the "HTTP Client" and "HTTP " spans into one for simplicity. All this is done in a new component inspired by https://github.com/opentracing-contrib/go-stdlib.
The result:
