fix(grpc): Return proper metadata object instead of list in… #3205
Conversation
fdellekart
changed the title
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #3205 +/- ##
==========================================
- Coverage 79.90% 79.87% -0.03%
==========================================
Files 137 137
Lines 15385 15387 +2
Branches 2611 2613 +2
==========================================
- Hits 12293 12290 -3
- Misses 2220 2223 +3
- Partials 872 874 +2
|
| metadata = ( | ||
| list(client_call_details.metadata) if client_call_details.metadata else [] | ||
| ) | ||
| client_call_details.metadata = client_call_details.metadata or Metadata() |
There was a problem hiding this comment.
Unfortunately, setting this attribute is not possible; see the following from our test logs:
tests/integrations/grpc/test_grpc_aio.py:76: in test_grpc_server_starts_transaction
await stub.TestServe(gRPCTestMessage(text="test"))
.tox/py3.8-grpc-latest/lib/python3.8/site-packages/grpc/aio/_interceptor.py:470: in __await__
call = yield from self._interceptors_task.__await__()
.tox/py3.8-grpc-latest/lib/python3.8/site-packages/grpc/aio/_interceptor.py:697: in _invoke
return await _run_interceptor(
.tox/py3.8-grpc-latest/lib/python3.8/site-packages/grpc/aio/_interceptor.py:671: in _run_interceptor
call_or_response = await interceptors[0].intercept_unary_unary(
sentry_sdk/integrations/grpc/aio/client.py:47: in intercept_unary_unary
client_call_details = self._update_client_call_details_metadata_from_scope(
sentry_sdk/integrations/grpc/aio/client.py:24: in _update_client_call_details_metadata_from_scope
client_call_details.metadata = client_call_details.metadata or Metadata()
E AttributeError: can't set attribute
Perhaps, ClientCallDetails provides some other way to set this?
There was a problem hiding this comment.
Thank you for pointing this out. I remembered there was a problem with setting some attribute and thought it was in add itself.
ClienCallDetails is a subclass of collections.namedtuple, which is immutable so it needs to be reconstructed to change a field value. This can be done with ._replace. Was hesitating to use it before as it is prefixed with _, however, according to the docs, this is just to avoid name clashes with fields.
Pushed a fixup, please have a look.
There was a problem hiding this comment.
Tests failing due to something else, will have a look.
There was a problem hiding this comment.
The metadata was not guaranteed to be an Optional[Metadata] object, as it is typed, up until grpcio 1.65.0 and a tuple was mistakenly passed on through the interceptors. This was fixed on the gRPC side.
Do you want to keep support for versions before that?
There was a problem hiding this comment.
Yes, we should keep support for anything we currently support, as this would be a breaking change that requires a major version bump if we drop support for any gRPC versions.
There was a problem hiding this comment.
Added the check as it is part of grpcio itself for newer versions 👍
There was a problem hiding this comment.
This is the relevant change in the grpc package
… client interceptor Fixes getsentry#2509
Up until version 1.65.0 of grpcio, the metadata was not guaranteed to arrive as the type specified in annotations but could be a tuple. To support versions before that we check and transform it here.
fdellekart
force-pushed
the
fix/grpc-aio-metadata-list
branch
from
fba9d83 to
ddba2a9
Compare
| client_call_details = client_call_details._replace(metadata=Metadata()) | ||
| elif not isinstance(client_call_details.metadata, Metadata): | ||
| client_call_details = client_call_details._replace( | ||
| metadata=Metadata.from_tuple(client_call_details.metadata) |
There was a problem hiding this comment.
What happens here if client_call_details.metadata is neither a tuple nor a Metadata object? Does Metadata.from_tuple handle the situation gracefully, or could we have an error in this situation?
If an error is possible, we need to make sure we handle it gracefully, even if this is a situation that is unlikely to occur. We need to be very careful with these kinds of edge cases in the SDK, since we never want to crash a user's application.
There was a problem hiding this comment.
Sorry for the late response, I've been distracted and forgot about this.
Looking at its implementation, the from_tuple method would work with any Iterable[Tuple[str, str]], although its name would suggest differently.
However, this could change any time with a new version of grpcio, so if we want to be save we could add an additional check if the input actually is a tuple. The same thing is done in the fix inside grpcio, so I think this would make sense to have here also to not break with future versions.
There was a problem hiding this comment.
Didn't yet push anything bc. I wanted to wait for your feedback, do you agree or have any further suggestions?
There was a problem hiding this comment.
Apologies @fdellekart, I missed your comment.
I just checked the ClientCallDetails class, and it looks like client_call_details.metadata is typed Optional[Metadata]. So, this entire elif block seems to be unnecessary, since if client_call_details.metadata is not None, then it is already a Metadata object.
Am I missing something here? Why did you add this elif block?
There was a problem hiding this comment.
As discussed previously in the other thread of this PR, older versions of grpc had a bug where this could also be a tuple although it was typed Optional[Metadata]. If grpcio < 1.65.0 should be supported (including the bug), then the elif block is necessary, else it can be removed, however, older versions before the fix will not be supported.
There was a problem hiding this comment.
Ah okay, I see the thread now. Then this change looks good. I am going to add a comment referencing the bug. It looks like the fix was added in 1.60.0, not 1.65.0, so I will reference 1.60.0 in the comment
There was a problem hiding this comment.
good catch, thx 🙂
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
4 similar comments
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
@szokeasaurusrex please get this merged if it's a valid change otherwise close it. Resetting the clock. |
github-actions
bot
removed
the
Trigger: tests using secrets
Fix wrong metadata type in async gRPC interceptor
Fixes #2509
The metadata of gRPC calls was transformed to a list in the client interceptor.
The list was compatible with gRPC itself as it is the format for metadata in the sync version, however it lead to downstream interceptors crashing as they expected a grpc.aio.Metadata object.
This PR now uses the expected type.