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.

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

fix(grpc): aio grpc client interceptor incorrect metadata handling #2363

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

legau
Copy link
Contributor

@legau legau commented Mar 20, 2024

Description

This fixes a bug when using grpcio>=1.60.0 and the gRPC telemetry. Introduced by grpc/grpc#34347 a tuple metadata is always converted to a Metadata object which follows the Python grpc docs and typing. In the current opentelemetry implementation there is a conversion to an OrderedDict which causes :

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = Metadata((('key', 'value'),)), key = ('key', 'value')

    def __getitem__(self, key: MetadataKey) -> MetadataValue:
        """When calling <metadata>[<key>], the first element of all those
        mapped for <key> is returned.
        """
        try:
>           return self._metadata[key][0]
E           KeyError: ('key', 'value')

This PR makes use of the Metadata object directly and propagates it as such.
This could cause regressions for some following interceptors handling metadata as tuples (same way this regression happened), I'm not sure if this would qualify as a breaking change though.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added metadata in grpc tests

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Mar 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@aabmass
Copy link
Member

aabmass commented Apr 4, 2024

Thanks for the PR @legau

This fixes a bug when using grpcio>=1.60.0 and the gRPC telemetry. Introduced by grpc/grpc#34347

Do you know if this change is backward compatible to older gRPC versions before v1.60.0?

@aabmass aabmass self-requested a review April 4, 2024 17:11
@legau
Copy link
Contributor Author

legau commented Apr 8, 2024

This transforms the tuple of <1.60 into a Metadata object so yes it is backwards compatible

@legau legau requested a review from a team April 12, 2024 10:16
@legau
Copy link
Contributor Author

legau commented Apr 12, 2024

I added the changelog entry

@lzchen lzchen merged commit 7656bdb into open-telemetry:main Apr 18, 2024
258 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants