fix(grpc): Return proper metadata object instead of list in… #3205
Conversation
fdellekart
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3205 +/- ##
==========================================
- Coverage 79.47% 79.39% -0.09%
==========================================
Files 133 133
Lines 14290 14292 +2
Branches 3003 3005 +2
==========================================
- Hits 11357 11347 -10
- Misses 2089 2103 +14
+ Partials 844 842 -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?
|
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 🥀 |
2 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 🥀 |
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.