open-telemetry / opentelemetry-python-contrib

OpenTelemetry instrumentation for Python modules
https://opentelemetry.io
Apache License 2.0
664 stars 550 forks source link

`aio` interceptor fails because `grpc.aio.Metadata` doesn't correctly implement `abc.Mapping` #2373

Open BatmanAoD opened 4 months ago

BatmanAoD commented 4 months ago

This is related to a known grpc bug: https://github.com/grpc/grpc/issues/26498

...however, I'm opening it here because that has been opened for several years, and this package could work around the issue.

Environment

I believe this applies to any environment using the current latest Python grpc and opentelemetry-instrumentation-grpc packages.

Steps to reproduce

Perform a gRPC call using aio, with non-empty metadata and a configured interceptor (e.g. using autoinstrumentation).

To see a simple demonstration that the code in propagate_trace_in_details doesn't work, just build an example Metadata object in a Python terminal and attempt to construct an OrderedDict from it:

>>> from collections import OrderedDict
>>> from grpc.aio import Metadata
>>> m = grpc.aio.Metadata({"foo", "bar"})
>>> OrderedDict(m)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<path>/.venv/lib/python3.9/site-packages/grpc/aio/_metadata.py", line 64, in __getitem__
    return self._metadata[key][0]
KeyError: ('bar', 'foo')

What is the expected behavior?

The call should succeed, and a corresponding trace span should be created.

What is the actual behavior?

There is a key error indicating that lookup of a metadata (key, value) pair failed.

BatmanAoD commented 4 months ago

My suggested workaround is to change this function: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_aio_client.py#L58

I believe it should construct mutable_metadata this way:

        mutable_metadata = OrderedDict()
        if metadata:
            # Constructing `OrderedDict` directly from `Metadata` fails due to this bug:
            # https://github.com/grpc/grpc/issues/26498
            for k, v in metadata:
                mutable_metadata[k] = v
BatmanAoD commented 4 months ago

...actually, aio.Metadata is already mutable. So this should probably just be:

        if not metadata:
            mutable_metadata = OrderedDict()
        elif isinstance(metadata, tuple):
            mutable_metadata = OrderedDict(metadata)
        else:
            mutable_metadata = metadata

...and then skip the tuple(...) below as well, if necessary.