census-instrumentation / opencensus-python

A stats collection and distributed tracing framework
Apache License 2.0
668 stars 250 forks source link

GoogleCloudPropagator header case sensitive #993

Open Marcalberga opened 3 years ago

Marcalberga commented 3 years ago

Describe your environment. Python 3.7 on conda environment. Falcon and Django frameworks. Gunicorn as wsgi. Local environment, still not deployed.

Steps to reproduce. Enable tracer.GoogleCloudPropagator and send a http request with the header X-CLOUD-TRACE-CONTEXT

What is the expected behavior? Http headers should be case-insensitive, and some services or frameworks uppercase all http headers received (or sent). The from_headers method should be case insensitive.

What is the actual behavior? Only exact name (case sensitive) is valid for the propagator to generate the span_context.

Can be found in opencensus/trace/propagation/google_cloud_format.py:21

Additional context. Working on a new falcon middleware copying the behaviour of flask middleware.

aabmass commented 3 years ago

Additional context. Working on a new falcon middleware copying the behaviour of flask middleware.

You are working on a Falcon middleware? Is it possible to handle this in the middleware with a case-insensitive dict? I think this issue could occur with other propagators as well. They seem to just expect a dict-like object for headers and call header.get().

Marcalberga commented 3 years ago

@aabmass , thanks for the quick reply. Here's a more detailed explanation of what we are going with.

On our microservices, we use falcon as a lightweight and fast framework (and enough for our needs). The middleware itself does not have much secret, just receives the requests and we use it's headers to create the spanContext.

The issue here is that gunicorn capitalizes all headers.

As a workaound, we have gone to something like

        for key, header in headers.items():
            if key.upper() == self._trace_header_key.upper():
                span_context = self._propagator.from_header(header)

        if not span_context:
            span_context = self._propagator.from_headers(headers)

There's a lot more of logic on building those span_context as we are receiving GCP PubSub and Cloud Tasks requests, which work different with headers, but the scope of the issue is here.

Anyway, as http_headers definition defines them as case insensitive, I think it would be good that propagators themselves work with this idea.

Also, as i'm reading source code from opencensus, it seems that other propagators may be facing same issue, and also flask middleware has no control over the capitalization of the headers. Django seems doing good as Django itself capitalizes in META.

aabmass commented 3 years ago

I think it would be good that propagators themselves work with this idea.

Also, as i'm reading source code from opencensus, it seems that other propagators may be facing same issue, and also flask middleware has no control over the capitalization of the headers.

Agreed, I am thinking if we can update the middlewares to pass a case-insensitive dictionary (like Django META) to the propagators, then we don't have to add that code to every propagator.