opentelemetry-python-contrib
opentelemetry-python-contrib copied to clipboard
Allow the user to cancel stream
Description
Currently, the user was not able to cancel the stream. The cause was that instead of returning _MultiThreadedRendezvous we returned the generator created from it.
I tried two options to fix this:
- In this WIP commit , I tried to end span by using add_done_callback. This kinda worked, but it was very hard to test using unittest. In tests, we either had to wait till thread handling events were already called before asserts or we could use the experimental single-threaded channel. Another problem was that the single-threaded channel did not handle the done callback when the stream was canceled. It handled correctly done callback when the stream ended or in case of error, but not during cancel. I think this is just a bug in that single-threaded implementation.
- The second option(and the one I picked) was wrapping
_MultiThreadedRendezvousin proxy, and ending span on all places where it was needed. Because this happened in the same thread, I did not have problems with unit testing.
The first solution is definitely less complicated from the point of implementation. However, I am not even sure if waiting for channel_spin is correct, spans in theory can be ended a little bit later. Compared to the second solution, we don't need to handle every place where the stream can end, we basically leave this to grpc implementation.
In the end, I picked the second solution mostly because it can be testable, it ends span almost immediately when the stream ends, and also because of that bug when done callback is not called when using the single threaded channel.
If you prefer the first solution or have some other in mind, let me know.
Fixes #2014
Type of change
Please delete options that are not relevant.
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
How Has This Been Tested?
- [ ] test_unary_stream_can_be_cancel - cancel unary stream in the middle of consuming responses
- [ ] test_stream_stream_can_be_cancel - cancel stream stream in middle of consuming responses
- [ ] test_finished_stream_cancel_does_not_change_status_of_span - try to cancel stream that already finished
Does This PR Require a Core Repo Change?
- [ ] Yes. - Link to PR:
- [x] No.
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.
- [x] Followed the style guidelines of this project
- [ ] Changelogs have been updated
- [x] Unit tests have been added
- [ ] Documentation has been updated