grpc-ecosystem / grpc-opentracing

OpenTracing is a set of consistent, expressive, vendor-neutral APIs for distributed tracing and context propagation
BSD 3-Clause "New" or "Revised" License
472 stars 98 forks source link

Add support for Python. #32

Closed rnburn closed 6 years ago

rnburn commented 7 years ago

This PR adds interceptors to support OpenTracing with Python. Unfortunately, gRPC doesn't provide any direct support for Python interceptors so the PR includes a submodule (grpcext) that implements interceptors on top of gRPC's Channel and Server APIs.

tedsuo commented 7 years ago

So glad to see Python support coming in. 👍

wkiser commented 7 years ago

How would you suggest integrating this with https://github.com/uber-common/opentracing-python-instrumentation?

Right now I am doing something like this (to get redis and sqlalchemy in the trace):

from opentracing_instrumentation.request_context import RequestContextManager
...
class CommandLine(command_line_pb2.CommandLineServicer):
    def Echo(self, request, context):
        with RequestContextManager(span=context.get_active_span()):
            _check_redis_cache()
            _make_db_query()
            return command_line_pb2.CommandResponse(text=request.text)

But I feel like there is a better solution.

rnburn commented 7 years ago

@wkiser There is a PR being worked on to add active span management to the Python OpenTracing API. Once that's in place, I'd expect the code to be updated to make use of it and in-process propagation between different frameworks would hopefully be more seamless. But I'm not sure what could be improved until that's done.

googlebot commented 7 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

evanj commented 7 years ago

Thanks for this! I totally stole the server interceptor and used it for an unrelated reason. Well, it is actually slightly related: I just want to log start/end of requests for debugging purposes. This is an extremely lightweight version of tracing :) worked like a charm. Looking forward to something like this landing in grpc proper.

lapointexavier commented 7 years ago

@rnburn out of curiosity, is there a timeline for when that PR would get merged (are there dependencies / blockers)?

tedsuo commented 7 years ago

@lapointexavier we were hoping that grpc would add python interceptors so that we wouldn't have to mutate the grpc objects. Given that that is in limbo, we should probably merge this.

lapointexavier commented 6 years ago

@rnburn @tedsuo My apologies, just wondering if this is still something that would get merged eventually? I'm working off a hand generated wheel, but it'd be nice to make it official, if it's in the plans.

rnburn commented 6 years ago

I think @bhs is the only one with write access to merge this -- were there any other concerns?

wkiser commented 6 years ago

I think people might be waiting on the PR for interceptors in gRPC to get merged.

That being said, I wouldn't mind this getting merged in the meantime since I am also running off of a hand generated wheel.

rnburn commented 6 years ago

I don't see any reason not to ship what we have and add support for gRPC's own interceptor's in a later version.

Even if that PR were to move today (which it won't), it would take a while for those changes to roll out to the various gRPC packages that most people are using, so having a version of this that can work with older version of gRPC makes a lot of sense to me.