Skip to content

fix(grpc): Return proper metadata object instead of list in… #3205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 5, 2024
Merged

fix(grpc): Return proper metadata object instead of list in… #3205

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c8fc77d
fix(grpc): Return propagate proper metadata object instead of list in…
fdellekart Jun 26, 2024
ddba2a9
fix(grpc): Transform metadata into Metadata object in case it's a tuple
fdellekart Jul 3, 2024
0d66f35
Merge branch 'master' into fix/grpc-aio-metadata-list
antonpirker Jul 18, 2024
0323fd7
docs(grpc): Add comment about workaround
szokeasaurusrex Dec 5, 2024
9563833
Merge remote-tracking branch 'origin/master' into fix/grpc-aio-metada…
szokeasaurusrex Dec 5, 2024
34725e8
Merge branch 'master' into fix/grpc-aio-metadata-list
szokeasaurusrex Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions sentry_sdk/integrations/grpc/aio/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
ClientCallDetails,
UnaryUnaryCall,
UnaryStreamCall,
Metadata,
)
from google.protobuf.message import Message

Expand All @@ -19,23 +20,19 @@ class ClientInterceptor:
def _update_client_call_details_metadata_from_scope(
client_call_details: ClientCallDetails,
) -> ClientCallDetails:
metadata = (
list(client_call_details.metadata) if client_call_details.metadata else []
)
if client_call_details.metadata is None:
client_call_details = client_call_details._replace(metadata=Metadata())
elif not isinstance(client_call_details.metadata, Metadata):
# This is a workaround for a GRPC bug, which was fixed in grpcio v1.60.0
# See https://github.com/grpc/grpc/issues/34298.
client_call_details = client_call_details._replace(
metadata=Metadata.from_tuple(client_call_details.metadata)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fdellekart fdellekart Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't yet push anything bc. I wanted to wait for your feedback, do you agree or have any further suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thx 🙂

)
for (
key,
value,
) in sentry_sdk.get_current_scope().iter_trace_propagation_headers():
metadata.append((key, value))

client_call_details = ClientCallDetails(
method=client_call_details.method,
timeout=client_call_details.timeout,
metadata=metadata,
credentials=client_call_details.credentials,
wait_for_ready=client_call_details.wait_for_ready,
)

client_call_details.metadata.add(key, value)
return client_call_details


Expand Down
Loading