Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Allow construction-time hooks to pass external tracing information to each request #348

Open
yurishkuro opened this issue Dec 23, 2015 · 4 comments

Comments

@yurishkuro
Copy link
Contributor

Scenario: an http server that is instrumented for Zipkin tracing and has a notion of the "current span". If such service makes a downstream call over TChannel, we don't want TChannel to start a new trace/span, but to start the new span as a child of the existing span established by the http server.

I am not sure if today it's possible to do this bridging by passing parent_tracing to each call(), but even if it's possible, it requires manually changing the application at every call site. Instead, it would be much cleaner if the app owner could set a certain "tracing hook" when creating the TChannel, so that this hook would get executed for every request and would retrieve the current parent span from http request context (by whatever means), and return the parent_tracing to the current request (or alternatively set it as the current parent_tracing in TChannel's RequestContext thread-local.

@abhinav
Copy link
Contributor

abhinav commented Dec 28, 2015

Turns out that it is currently possible to do this with a non-public API: request_context from https://github.com/uber/tchannel-python/blob/master/tchannel/context.py. We have 3 different Trace objects, though (we're working on fixing that). @junchaoWu will be able to answer which one you're supposed to use with that. For the short term, this will probably suffice.

However, we would also like TChannel servers to be able to inject their own tracing information into HTTP clients' requests to get the full picture. Do you think it's reasonable to have a separate small library that basically provides the following APIs.

get_current_trace()

with current_trace(trace):
    # ...

And then having both, TChannel and the HTTP server/client use that?

CC @blampe @breerly

@yurishkuro
Copy link
Contributor Author

The "small library" that you want to use is Open Tracing API - https://github.com/opentracing. There is a python version here - https://github.com/uber-common/opentracing-python, it will be open-sourced soon.

The trace context propagation doesn't exist in that version yet, but does exist in our internal jaeger-client-python repo, which also supports instrumentation of HTTP call sites. However, in any case, the method of propagating the context is always going to be somewhat specific to a particular instrumentation library, and it's better not to monkey patch stuff if we can avoid it, so I would still recommend having a construction-time hook in TChannel where application can register a functor that will return current span, retrieved by whatever means are appropriate for the current instrumentation library. E.g. with Jaeger instrumentation, that function already exists:

def get_current_span():
    """
    :return: Returns current span associated with the current stack context, or None.
    """
    return TraceContextManager.current_span()

@yurishkuro
Copy link
Contributor Author

@abhinav @blampe resuming this thread. This is how I had to do a manual bridge from OpenTracing span to TChannel span

    # retrieve current OpenTracing Span
    span = get_current_span()  
    # rely on the fact that the field names are the same, 
    # and pretend that the OpenTracing span's context is the parent
    with tchannel_context.request_context(parent_tracing=span.trace_context):
        return channel.json(
            service='tchannel-server',
            endpoint='endpoint',
            body={
                'req': 'body'  # dummy body
            },
            headers={
                'req': 'header'  # dummy headers
            },
        )

It works, but it needs to be done on every client call site. I would prefer a static binding so that the code above could be executed in a hook I can attach to the TChannel object. However, the existing "hooks" mechanism is too late for that.

@blampe
Copy link
Contributor

blampe commented Jan 27, 2016

@junchaoWu could we just swap out our zipkin implementation with the open tracing lib?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants