threading integration cannot be used with NoopTracer

ewhauser commented 5 years ago

The NoopTracer does not implement sampler, exporter, or propagator but the threading integration assumes that the tracer available has those properties. This can either be fixed by adding the properties to NoopTracer or making the threading integration check for them explicitly.

Happy to submit a fix with some guidance on direction.

ewhauser commented 5 years ago

Actually, the real issue here is that the Tracer implementation does not pass across the thread boundary. So when using the threading module, it always instantiates the base Tracer instance - effectively ignoring NoopTracer.

hongrich commented 5 years ago

I believe I've also encountered the same issue. Here is an example traceback and a reproducible example in case this is helpful:

Traceback (most recent call last):
  File "/env/lib/python3.7/site-packages/gunicorn/arbiter.py", line 583, in spawn_worker
    worker.init_process()
  File "/env/lib/python3.7/site-packages/gunicorn/workers/gthread.py", line 104, in init_process
    super(ThreadWorker, self).init_process()
  File "/env/lib/python3.7/site-packages/gunicorn/workers/base.py", line 134, in init_process
    self.run()
  File "/env/lib/python3.7/site-packages/gunicorn/workers/gthread.py", line 211, in run
    callback(key.fileobj)
  File "/env/lib/python3.7/site-packages/gunicorn/workers/gthread.py", line 132, in accept
    self.enqueue_req(conn)
  File "/env/lib/python3.7/site-packages/gunicorn/workers/gthread.py", line 122, in enqueue_req
    fs = self.tpool.submit(self.handle, conn)
  File "/env/lib/python3.7/site-packages/opencensus/trace/ext/threading/trace.py", line 126, in call
    wrapped_kwargs["sampler"] = _tracer.sampler
AttributeError: 'NoopTracer' object has no attribute 'sampler'

Can be reproduced by running gunicorn with gthread worker class:

gunicorn main:app --worker-class gthread

with the following in a file called main.py

from opencensus.trace import config_integration

config_integration.trace_integrations(["threading"])

def app(environ, start_response):
    """Simplest possible application object"""
    data = b"Hello, World!\n"
    status = "200 OK"
    response_headers = [
        ("Content-type", "text/plain"),
        ("Content-Length", str(len(data))),
    ]
    start_response(status, response_headers)
    return iter([data])

(This is just the example take from gunicorn documentation: http://docs.gunicorn.org/en/latest/run.html)

reyang commented 5 years ago

@ewhauser @c24t I think probably we would want to move tracer out of the context?

Is there a common scenario where people want to use different samplers/propagators/exporters based on the context?
How do we align with OpenCensus Java / Go / etc.?

ewhauser commented 5 years ago

I’m not sure how this is handled in other libraries, but the scenario for me was pretty simple:

I have a flag that I pass to my application that enables or disables tracing
If tracing is disabled, I use the NoopTracer. If enabled, I use Stackdriver

On Thu, Apr 4, 2019 at 5:07 PM Reiley Yang notifications@github.com wrote:

@ewhauser https://github.com/ewhauser @c24t https://github.com/c24t I think probably we would want to move tracer out of the context?

Is there a common scenario where people want to use different samplers/propagators/exporters based on the context?

How do we align with OpenCensus Java / Go / etc.?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/census-instrumentation/opencensus-python/issues/442#issuecomment-480105654, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIBPZw7T4MdyDHAeVrkZaR6O7v1JfKoks5vdpPFgaJpZM4Z9Th1 .

colincadams commented 5 years ago

One of the confusing things to me when reading the trace code is that there are two different objects called Tracer.

opencencus.trace.tracer.Tracer
- attributes: sampler, exporter, propagator, tracer
opencensus.trace.tracers.base.Tracer
- context_tracer.ContextTracer, noop_tracer.NoopTracer inherit from it

Typically the first opencensus.trace.tracer.Tracer is put in the execution_context as is done by the Tracer.store_tracer() method on that object. However by default the execution_context holds a NoopTracer.

reyang commented 5 years ago

Typically the first opencensus.trace.tracer.Tracer is put in the execution_context as is done by the Tracer.store_tracer() method on that object. However by default the execution_context holds a NoopTracer.

@colincadams This is also a big headache for @c24t and me. It is something that we need to refactor before calling it v1.0. I do have some idea on how to refactor it, let me know if you have some ideas or want to work on this.

IamJeffG commented 4 years ago

Just confirming that this is still a bug for us -- and in fact it's a big enough problem that it's leading to our having to abandon the threading integration altogether. (Which is a shame because without visibility into work done in threads, we lose very many spans and lose a lot of utility of opencensus in general.)

Can be reproduced by running gunicorn with gthread worker class: gunicorn main:app --worker-class gthread

I'm curious about the interaction with gunicorn's worker here, because (similar to https://github.com/census-instrumentation/opencensus-python/issues/806) we don't see this bug when using flask run, only with gunicorn. Is there a gunicorn worker class that doesn't trigger this bug?

aberres commented 4 years ago

Our of curiosity: Is this taken care of with open-telemetry?

lzchen commented 4 years ago

Those fields are no longer part of the Tracer.

Sampler is part of TracerProvider. Exporter is part of SpanProcesser which is in turn part of TracerProvider

Propagator is set globally or instantiated manually

IamJeffG commented 4 years ago

@lzchen does your comment imply that this bug is fixed? Do you know which is the first version to have the fix?

lzchen commented 4 years ago

@c24t

census-instrumentation / opencensus-python

threading integration cannot be used with NoopTracer #442