Add support for Python. #32

rnburn · 2017-05-09T16:17:23Z

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.

…C call.

tedsuo · 2017-05-10T01:11:24Z

So glad to see Python support coming in. 👍

bhs

Sorry for the insane delay – did not know this was here. :-/

bhs · 2017-05-27T21:25:10Z

python/README.md

+
+```python
+tracer = # some OpenTracing Tracer instance
+interceptor = open_tracing_client_interceptor(tracer)


why is there this helper function instead of just initializing an OpenTracingClientInterceptor directly?

This was to be consistent with the rest of gRPC where the __init__.py file only exposes interface classes and uses functions to construct concrete classes. (See for example the server function that does the same thing.)

Not entirely sure, but I would guess the reasons for this are to keep the documentation in a single location and to have a convention that allows easily supporting additional constructor functions if needed.

bhs · 2017-05-27T21:25:16Z

python/README.md

+
+```python
+tracer = # some OpenTracing Tracer instance
+interceptor = open_tracing_server_interceptor(tracer)


bhs · 2017-05-27T21:25:38Z

python/README.md

+
+### Server-side usage example
+
+```python


let's include import statements in these two examples

I added import statements.

bhs · 2017-05-27T21:27:07Z

python/examples/integration/README.md

+
+## Usage
+```
+python integration_server.py --access_token `lightstep-access-token` &


would be better to support Jaeger's (OSS) library here... it can run in a single local docker image.

Ok. Will switch the examples to use Jaeger.

All the examples use Jaeger now.

bhs · 2017-05-27T21:27:33Z

python/examples/store/README.md

+
+## Usage
+```
+python store_server.py --access_token `lightstep-access-token` &


(same comment about Jaeger)

bhs · 2017-05-29T21:35:21Z

python/examples/integration/integration_client.py

+import argparse
+
+import grpc
+from jaeger_client import Config


doesn't some sort of dependency need to be added to setup.py or similar for this to work?

I added a requirements.txt file in the example directory that includes jaeger-client as an dependency.

bhs · 2017-05-29T21:40:21Z

python/grpc_opentracing/_server.py

+# updates the span accordingly.
+def _check_error_code(span, servicer_context):
+    if servicer_context.code != grpc.StatusCode.OK:
+        span.set_tag('error', True)


nit: this is defined symbolically here: https://github.com/opentracing/opentracing-python/blob/master/opentracing/ext/tags.py#L47

I changed to use the constants in opentracing.ext.tags available in opentracing-1.2.2, but tags.ERROR was added for opentracing-1.2.3 which hasn't been released yet.

bhs · 2017-05-29T21:42:32Z

python/grpc_opentracing/__init__.py

+    """Creates a service-side interceptor that can be use with gRPC to add
+    OpenTracing information.
+
+  Args:


missing a comment for traced_attributes

bhs · 2017-05-29T21:44:13Z

python/grpc_opentracing/__init__.py

+def open_tracing_client_interceptor(tracer,
+                                    active_span_source=None,
+                                    log_payloads=False,
+                                    traced_attributes=None):


no comment provided for this param

tracer_attributes is documented now.

thinking more deeply about this, IMO it might be better to express this functionality as a SpanDecorator function that gives the caller the flexibility to modify the RPC Span in any way (and provides RPC context information like headers, method type/name, deadline, etc in the decorator function arguments somehow).

Sure. I'll make that change. It was mostly done this way just to match the Java version.

I removed the traced_attributes options and added a span_decorator that can be used instead.

wkiser · 2017-06-19T15:50:03Z

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 · 2017-06-19T16:10:18Z

@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 · 2017-06-19T16:10:29Z

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 · 2017-08-11T21:50:34Z

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 · 2017-09-22T15:25:43Z

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

tedsuo · 2017-09-22T21:58:56Z

@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 · 2017-11-03T18:17:07Z

@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 · 2017-11-03T23:07:45Z

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

wkiser · 2017-11-04T14:55:25Z

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 · 2017-11-04T23:03:18Z

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.

bhs

Merging seems like a definite improvement over non-merging. :) Looking forward to a core GRPC API that makes interceptors cleaner in python, though. Thanks!

Ryan Burn and others added 30 commits March 9, 2017 00:03

Move in Python code

943601e

Renamed otgrpc module to grpc_opentracing

2e47f33

Added a README file

1e2c8e5

Provide access to the active span on the server-side

69a0f14

Renamed test/service.py to test/_service.py

e15c6bb

Added OpenTracing tests.

49aecf3

Added integration section to README

e2ed4f4

added trivial example

fefa8f3

Minor changes to store example

8ed5298

Added an example of how to integrate gRPC with other spans.

d573779

Added READMEs for the examples

5993989

Removed superfluous statement.

c1542f9

Reworded README.

f4dfb11

Create a third span when realizing the future from an asynchronous RP…

74cc989

…C call.

Added additional tests for erroring RPCs.

fa3ebeb

Removed unnecessary class.

32e890f

Added additional tags that can be optionally traced.

edf689b

Detect errors on the service-side if they're reported via .

257ace7

Minor refactoring.

b6832c4

Changed interceptor API to pass timeout information on the client-side.

0fc1e50

Added grpc.deadline_millis as an optional span tag.

a26f66c

Corrected comment formatting.

565d075

Changed terminology to be more consistent with gRPC.

3936faf

Added a test case for RPCs that error with exceptions.

28fba33

Added interoperability tests.

af32e5b

Added metadata tests.

cd757ad

Added tests for asyc RPCs.

f914211

Tweaked get_deadline_millis function.

3f897e8

Added an example for the deadline_millis tag.

377bb5f

Changed interceptor API to support accessing request values.

3e8bd44

Ryan Burn and others added 2 commits March 21, 2017 18:25

gRPC changed to indent with 4 spaces, so updating accordingly.

f1583d1

Changed asynchronous RPCs to use two spans.

a4a0af6

tedsuo mentioned this pull request May 10, 2017

Create an "interceptor" mechanism for GRPC Python grpc/grpc#8767

Closed

bhs reviewed May 27, 2017

View reviewed changes

rnburn added 4 commits May 29, 2017 14:04

Added import statements to README.md.

c66774e

Changed trivial example to use Jaeger's tracer.

830a645

Convert integration example to use Jaeger's tracer.

4be60a0

Convert store example to use Jaeger.

db642c2

bhs reviewed May 29, 2017

View reviewed changes

rnburn added 5 commits May 29, 2017 20:10

Added requirements.txt file for examples.

d4550ef

Document traced_attribute arguments.

6f88adc

Use constants in opentracing.ext.tags.

752f76a

Replace client-side, optional attributes with a span decorator.

ea11588

Replace optional server-side attributes with a span decorator.

1acf007

rnburn added 2 commits September 26, 2017 14:08

Add missing test dependency.

1f78472

Add new ServicerContext methods added by grpc.

23d3f79

bhs approved these changes Nov 5, 2017

View reviewed changes

bhs merged commit 01f8541 into grpc-ecosystem:master Nov 5, 2017

c24t mentioned this pull request Mar 25, 2020

gRPC integration open-telemetry/opentelemetry-python#476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Python. #32

Add support for Python. #32

rnburn commented May 9, 2017

tedsuo commented May 10, 2017

bhs left a comment

bhs May 27, 2017

rnburn May 27, 2017

bhs May 27, 2017

bhs May 27, 2017

rnburn May 27, 2017

rnburn May 29, 2017

bhs May 27, 2017

rnburn May 27, 2017

rnburn May 29, 2017

bhs May 27, 2017

bhs May 29, 2017

rnburn May 30, 2017

bhs May 29, 2017

rnburn May 30, 2017

bhs May 29, 2017

bhs May 29, 2017

rnburn May 30, 2017

bhs May 30, 2017

rnburn May 30, 2017

rnburn May 31, 2017

wkiser commented Jun 19, 2017

rnburn commented Jun 19, 2017

googlebot commented Jun 19, 2017

evanj commented Aug 11, 2017

lapointexavier commented Sep 22, 2017

tedsuo commented Sep 22, 2017

lapointexavier commented Nov 3, 2017

rnburn commented Nov 3, 2017

wkiser commented Nov 4, 2017

rnburn commented Nov 4, 2017 •

edited

Loading

bhs left a comment

Add support for Python. #32

Add support for Python. #32

Conversation

rnburn commented May 9, 2017

tedsuo commented May 10, 2017

bhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkiser commented Jun 19, 2017

rnburn commented Jun 19, 2017

googlebot commented Jun 19, 2017

evanj commented Aug 11, 2017

lapointexavier commented Sep 22, 2017

tedsuo commented Sep 22, 2017

lapointexavier commented Nov 3, 2017

rnburn commented Nov 3, 2017

wkiser commented Nov 4, 2017

rnburn commented Nov 4, 2017 • edited Loading

bhs left a comment

Choose a reason for hiding this comment

rnburn commented Nov 4, 2017 •

edited

Loading