jaegertracing / jaeger-client-python

šŸ›‘ This library is DEPRECATED!
https://jaegertracing.io/
Apache License 2.0
408 stars 155 forks source link

Use if isinstance dict has issues with requests.structures.CaseInsensitiveMapping #302

Closed electrofelix closed 2 years ago

electrofelix commented 3 years ago

Requirement - what kind of business use case are you trying to solve?

Use jaeger tracing with recent requests library to trace results in a Traceback that looks like:

Traceback (most recent call last):
.... <pruned some internal code references>
  File "/usr/local/lib/python3.8/site-packages/opsramp/api.py", line 48, in search
    return super(ORapi, self).get(suffix, headers)
  File "/usr/local/lib/python3.8/site-packages/opsramp/base.py", line 335, in get
    return self.api.get(suffix, headers=headers)
  File "/usr/local/lib/python3.8/site-packages/opsramp/base.py", line 290, in get
    return self.process_result(url, resp)
  File "/usr/local/lib/python3.8/site-packages/opsramp/base.py", line 280, in process_result
    return self.collate_pages(resp.request, data=data)
  File "/usr/local/lib/python3.8/site-packages/opsramp/base.py", line 210, in collate_pages
    next_page = self.session.get(
  File "/usr/local/lib/python3.8/site-packages/requests/sessions.py", line 555, in get
    return self.request('GET', url, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/requests_opentracing/tracing.py", line 35, in request
    self._get_tracer().inject(span.context, Format.HTTP_HEADERS, headers)
  File "/usr/local/lib/python3.8/site-packages/jaeger_client/tracer.py", line 267, in inject
    codec.inject(span_context=span_context, carrier=carrier)
  File "/usr/local/lib/python3.8/site-packages/jaeger_client/codecs.py", line 288, in inject
    raise InvalidCarrierException('carrier not a dictionary')
opentracing.propagation.InvalidCarrierException: carrier not a dictionary

Problem - what in Jaeger blocks you from solving the requirement?

It appears that the client library is performing checks to ensure it is being passed a subclass of dict which fails due to requests using a custom class to handle headers being case-insensitve.

Possibly it should switch checking against collections.abc.MutableMapping which dict() would satisfy or possibly check that carrier satisfies hasattr(x, "items").

Proposal - what do you suggest to solve the problem or improve the existing situation?

Switch the check in the inject behaviour https://github.com/jaegertracing/jaeger-client-python/blob/7984b2872b09a36b7ea0ae6479035ed61666af30/jaeger_client/codecs.py#L57 to be against collections.abc.MutableMapping or match the extract behaviour at https://github.com/jaegertracing/jaeger-client-python/blob/7984b2872b09a36b7ea0ae6479035ed61666af30/jaeger_client/codecs.py#L92.

Any open questions to address

Does it make sense to simply replace the existing isinstance(carrier, dict) with isinstance(carrier, collections.abc.MutableMapping) or would it be preferred to use hasattr(carrier, "items") to have inject match extract.

Given that there is also a check of isinstance(references, list) at https://github.com/jaegertracing/jaeger-client-python/blob/7984b2872b09a36b7ea0ae6479035ed61666af30/jaeger_client/tracer.py#L161, which seems a little more difficult to replace as bytearray would satisfy abc.MutableSequence though list would not satisfy isinstance(..., abc.ByteString) which could be used to distinguish if needed.

myobj = []
not isinstance(myobj, abc.ByteString) and isinstance(myobj, abc.MutableSequence)

returns True

myobj = bytearray()
not isinstance(myobj, abc.ByteString) and isinstance(myobj, abc.MutableSequence)

results in False

It's not clear that use of the collections.abc is perfect.

Originally found by my colleague @davemcn