Add a create_http_headers_for_my_span() zipkin_span method

[dbishop]

The rationale is that I wanted to create a Kind=CLIENT span in server A
with a new span_id (and parent_span_id of the span that had been active
at the time), then generate headers with that same new span_id for the
request to server B, where the span there would have shared=True in
its local-root-span.

So that gives me 2 spans total, the parent span, and a single shared
span with cs/cr happening on server A and sr/ss happening on server B.

If I'd just used create_http_headers_for_new_span(), then I'd have had
three total spans: the parent span, a weird span with only cs/cr from
server A, and a 3rd span with only sr/ss from server B.

I'm not sure if not having this caused some of the confusion in
Issue #126 or not...

[coveralls]

Coverage remained the same at ?% when pulling 46bc0b9 on swiftstack:create-http-headers-for-THIS-span into 86a2d7c on Yelp:master.

[dbishop]

As noted, just now, in Issue #144 , I'm not sure the API for the new function is ideal, as currently represented in this PR. I plan on working on this PR a bit more, so please don't merge it yet.

[dbishop]

Like, maybe this should be more like an instance method on zipkin_span:

diff --git a/py_zipkin/zipkin.py b/py_zipkin/zipkin.py
index 12c8f9f..3c35f21 100644
--- a/py_zipkin/zipkin.py
+++ b/py_zipkin/zipkin.py
@@ -618,6 +618,29 @@ class zipkin_span(object):
         if self.logging_context:
             self.logging_context.span_name = name
 
+    def create_http_headers_for_my_span(self):
+        """
+        Generate the headers for sharing this context object's zipkin_attrs
+        with a shared span on another host.
+
+        If this instance doesn't have zipkin_attrs set, for some reason, an
+        empty dict is returned.
+
+        :returns: dict containing (X-B3-TraceId, X-B3-SpanId, X-B3-ParentSpanId,
+                    X-B3-Flags and X-B3-Sampled) or an empty dict.
+        """
+        zipkin_attrs = self.zipkin_attrs
+        if not zipkin_attrs:
+            return {}
+
+        return {
+            'X-B3-TraceId': zipkin_attrs.trace_id,
+            'X-B3-SpanId': zipkin_attrs.span_id,
+            'X-B3-ParentSpanId': zipkin_attrs.parent_span_id,
+            'X-B3-Flags': '0',
+            'X-B3-Sampled': '1' if zipkin_attrs.is_sampled else '0',
+        }
+

[dbishop]

...with usage as

        span_ctx = zipkin_client_span(...)
        span_ctx.start()
        span_ctx.add_remote_endpoint(...)
        b3_headers = span_ctx.create_http_headers_for_my_span()
        [inject b3_headers into outbound HTTP RPC call]

[drolando]

Just 2 very small changes then it's good to go

[drolando]

Can you make this a regular function that receives ZipkinAttrs as argument? That way it can be reused more widely even if you don't have access to the span (e.g. you can get the zipkin_attrs from the tracer)

And if you think that having this extra method on the span context is useful you can keep it and call create_http_headers_for_my_span(self. zipkin_attrs)

[dbishop]

@drolando So... confession time.

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]

too much indentation. Please rebase on the black branch and run tox -e black to reformat this code

[dbishop]

@drolando I'm going to have to think about this one more...

I'd love to hear your thoughts on how best to allow access to some notion of "current open zipkin_span() context instance" without a memory leak or other awkwardness.

I think our current instrumentation of OpenStack Swift doesn't need to do that at the moment, so I'm out of the woods.