grpc: allow the user to cancel stream v2#3823
grpc: allow the user to cancel stream v2#3823aidandj wants to merge 13 commits intoopen-telemetry:mainfrom
Conversation
- set_attribute and also set_status are not called if span was already ended. Simple reorder of function calls helped - I also realized that accessing end_time is dangerous, in SDK it is protected by lock. I solved this by remembering if we already called end. Calling multiple times end on span is not problematic, but it generates warning logs and is probably not good practice - updated CHANGELOG.md - fix lint
This reverts commit 5b26e33
This reverts commit d0abacf
|
|
aidandj
force-pushed
the
aidan/allow-the-user-to-cancel-stream-v2
branch
from
17c89a7 to
6ed2e7d
Compare
aidandj
force-pushed
the
aidan/allow-the-user-to-cancel-stream-v2
branch
from
6ed2e7d to
9bdf76a
Compare
xrmx
changed the title
xrmx
moved this from Ready for review
to Reviewed PRs that need fixes
in Python PR digest
xrmx
moved this from Reviewed PRs that need fixes
to Ready for review
in Python PR digest
Signed-off-by: Aidan Jensen <aidandj.github@gmail.com>
aidandj
force-pushed
the
aidan/allow-the-user-to-cancel-stream-v2
branch
from
b6cf2e3 to
979f0c6
Compare
|
Friendly new years bump on this one. |
|
bump |
|
It would be amazing if this could be merged. Not being able to cancel when instrumented by OTel is problematic. |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
Not stale |
| }, | ||
| ) | ||
|
|
||
| def test_stream_stream_can_be_cancel(self): |
There was a problem hiding this comment.
Nit:
| def test_stream_stream_can_be_cancel(self): | |
| def test_stream_can_be_canceled(self): |
| try: | ||
| future.result() | ||
| except grpc.FutureCancelledError: | ||
| span_.set_status(Status(StatusCode.OK)) |
There was a problem hiding this comment.
I think span status as OK makes sense since the choices are unset, ok, or error. Would be good to get others' thoughts on this. The important part is this follows semconv for rpc status code.
There was a problem hiding this comment.
I don't believe that canceling a span from the server side should be considered an error, although it may make sense to look at what another language does.
tammy-baylis-swi
left a comment
tammy-baylis-swi
left a comment
There was a problem hiding this comment.
Thank you for this and your patience. Please could you resolve the merge conflicts.
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
tammy-baylis-swi
moved this from Ready for review
to Reviewed PRs that need fixes
in Python PR digest
|
This PR has been closed due to inactivity. Please reopen if you would like to continue working on it. |
github-project-automation
Bot
moved this from Reviewed PRs that need fixes
to Done
in Python PR digest
5 participants
Description
An updated version of this PR: #2093
Fixes # (issue)
#2014
Type of change
Please delete options that are not relevant.
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.