Yelp / py_zipkin

Provides utilities to facilitate the usage of Zipkin in Python
Apache License 2.0
224 stars 47 forks source link

How to access current zipkin_span context object #150

Open drolando opened 4 years ago

drolando commented 4 years ago

Question from @dbishop in #142

I also wanted access to the "current zipkin_span context object" from anywhere (remember, our use-case is eventlet which is very "this code appears to be running isolated to just handling one request" even though it may be spread out all over the place). So we have THIS hack that I haven't even bothered trying to get you to accept, haha:

_tls = threading.local()  # GREEN-thread local storage for a SpanSavingTracer

class SpanSavingTracer(Tracer):
    """
    Like py-zipkin's Tracer, but supports accessing the "current" zipkin span
    context object.
    """
    def __init__(self):
        super(SpanSavingTracer, self).__init__()
        self._span_ctx_stack = Stack()

    def get_span_ctx(self):
        return self._span_ctx_stack.get()

    def push_span_ctx(self, ctx):
        self._span_ctx_stack.push(ctx)

    def pop_span_ctx(self):
        return self._span_ctx_stack.pop()
...
def get_default_tracer():
    """Return the current default Tracer.

    For now it'll get it from thread-local in Python 2.7 to 3.6 and from
    contextvars since Python 3.7.

    :returns: current default tracer.
    :rtype: Tracer
    """
    if not hasattr(_tls, 'tracer'):
        _tls.tracer = SpanSavingTracer()
    return _tls.tracer
...
class ezipkin_span(zipkin_span):
...
    def start(self):
        # retval will be same as "self" but this feels a little cleaner
        retval = super(ezipkin_span, self).start()
        if retval.do_pop_attrs:
            self.get_tracer().push_span_ctx(retval)

        return retval

    def stop(self, _exc_type=None, _exc_value=None, _exc_traceback=None):
        if self.do_pop_attrs:
            self.get_tracer().pop_span_ctx()
        return super(ezipkin_span, self).stop(_exc_type=_exc_type,
                                              _exc_value=_exc_value,
                                              _exc_traceback=_exc_traceback)

I feel dirty even pasting that in here.

I'm pretty sure that's trying to leak memory and what should probably change is for the zipkin_span object's _tracer instance be made a weak reference.

BUT, I obviously haven't really thought this through, nor tried to measure memory leakage, nor seeing if the weakref thing will break something.

So... all this is to say that I want access to the zipkin_span instance anyway, and that's why this patch looks the way it does.

Can you render an opinion on the above? Please tell me you can think of a clean way to get me what i want? 😅

drolando commented 4 years ago

I don't really have a better suggestion on how to do this. The only global variable that we maintain is the tracer, so the context has to be stored in there. As you show in your example this feels very similar to the current context_stack containing the zipkin_attrs. In the same way we'd need to push a new span as soon as we enter it and pop it out when we leave it.

The tracer already has a clear() method that we already call to wipe its content, so if your SpanSavingTracer had a custom clear function that also wipes the other stack you shouldn't have memory leaks.


if not hasattr(_tls, 'tracer'):
        _tls.tracer = SpanSavingTracer()

I don't think this is necessary. If when you start your application you immediately create your SpanSavingTracer and call set_default_tracer(tracer) then everything should just use your custom tracer.

dbishop commented 4 years ago

I ran into an issue with my approach for tracing (some) HTTP requests made by Swift code: in some cases, an HTTP request is started and headers sent in a child greenthread, but the body-reading and connection-closing happens in the child's parent greenthread. That violates a basic assumption that my tracing (the code on top of py_zipkin) has, which is that a span is opened and closed within a single greenthread.

That's not really relevant to you or py_zipkin--that's our cross to bear. But my point in bringing it up is that I don't have time to tackle that problem right in our tracer usage now, so I won't be active here for a bit until our tracing bumps up in priority again.

Thanks for working with me to get those patches accepted, and I hope you and other users find them as helpful as I do!