Skip to content
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

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Changes from 2 commits
Commits
Show all changes
3 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
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
21 changes: 8 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 @@ -20,20 +21,14 @@ 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):
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?

)
for key, value in Scope.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