Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a create_http_headers_for_my_span() zipkin_span method #142

Closed
wants to merge 1 commit into from
+65 −0
Closed

Add a create_http_headers_for_my_span() zipkin_span method #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions py_zipkin/zipkin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,29 @@ def override_span_name(self, name):
if self.logging_context:
self.logging_context.span_name = name

def create_http_headers_for_my_span(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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? 😅

"""
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',
}


def _validate_args(kwargs):
if 'kind' in kwargs:
Expand Down
42 changes: 42 additions & 0 deletions tests/zipkin_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,48 @@ def test_override_span_name(self):
assert span.span_name == 'new_name'
assert span.logging_context.span_name == 'new_name'

def test_create_http_headers_for_my_span_no_zipkin_attrs(self):
with zipkin.zipkin_client_span(
service_name='test_service',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

span_name='test_span',
transport_handler=MockTransportHandler(),
sample_rate=100.0,
) as span:
# Not sure how this could ever happen in real life, but if it
# did...
span.zipkin_attrs = None
assert {} == span.create_http_headers_for_my_span()

def test_create_http_headers_for_my_span_is_sampled(self):
with zipkin.zipkin_client_span(
service_name='test_service',
span_name='test_span',
transport_handler=MockTransportHandler(),
sample_rate=100.0,
) as span:
assert {
'X-B3-TraceId': span.zipkin_attrs.trace_id,
'X-B3-SpanId': span.zipkin_attrs.span_id,
'X-B3-ParentSpanId': span.zipkin_attrs.parent_span_id,
'X-B3-Flags': '0',
'X-B3-Sampled': '1',
} == span.create_http_headers_for_my_span()

def test_create_http_headers_for_my_span_is_NOT_sampled(self):
with zipkin.zipkin_client_span(
service_name='test_service',
span_name='test_span',
transport_handler=MockTransportHandler(),
sample_rate=0.0,
) as span:
assert {
'X-B3-TraceId': span.zipkin_attrs.trace_id,
'X-B3-SpanId': span.zipkin_attrs.span_id,
'X-B3-ParentSpanId': span.zipkin_attrs.parent_span_id,
'X-B3-Flags': '0',
'X-B3-Sampled': '0',
} == span.create_http_headers_for_my_span()


def test_zipkin_client_span():
context = zipkin.zipkin_client_span('test_service', 'test_span')
Expand Down