Add Implementation of the open-tracing interceptor using gRPC interceptor. by zhyon404 · Pull Request #14 · opentracing-contrib/python-grpc

zhyon404 · 2018-09-30T17:55:21Z

No description provided.

rnburn · 2018-10-01T16:12:02Z

Could we remove the existing interceptor code? I don't think we'd want two implementations.

zhyon404 · 2018-10-02T04:54:55Z

We could, of course this will loss backward compatible, but I agree that only one implementation is better. Do you want me to do this?

…ptor. (opentracing-contrib#13)

rnburn · 2018-10-04T17:52:25Z

Yes, we can bump the major version to indicate it's not backward compatible.

wenbochang · 2018-10-15T19:02:09Z

Thank you so much for this!

…n-grpc # Conflicts: # README.md

zhyon404 · 2018-10-31T05:35:26Z

basically done. not writing much test though

carlosalberto · 2019-01-11T07:13:40Z

          command: |
            . venv/bin/activate
-            python setup.py test
+            python3 setup.py test


Do we really need to run Python 3 here?

I tried, using python works now.

carlosalberto · 2019-01-11T07:14:27Z


-See the below code for basic usage or [examples/trivial](examples/trivial) for a
-complete example.
+using official grpc interceptor


I think we could leave the original line ("See the below code...") and also mention this new note at the end of the paragraph ;)

carlosalberto · 2019-01-11T07:15:04Z

+tracer_interceptor = client_interceptor.OpenTracingClientInterceptor(tracer)
 channel = # the grpc.Channel you created to invoke RPCs
-channel = intercept_channel(channel, interceptor)
+channel = grpc.intercept_channel(channel, tracer_interceptor)


carlosalberto · 2019-01-11T07:16:36Z

-
-On the server-side, the `context` argument passed into your service methods
-packages the gRPC span created on the server-side.
+from grpc_opentracing import scope


I have mixed feelings about this, as this will probably get discarded anyway in favor of the OT 2.0 API (ScopeManager)...

I looked into current OT ScopeManager when I wrote this, and thinking it not meet the need. If we can use OT 2.0 API (ScopeManager) would be great.

carlosalberto · 2019-01-11T07:21:10Z

    author='LightStep',
    license='',
-    install_requires=['opentracing>=1.2.2', 'grpcio>=1.1.3', 'six>=1.10'],
+    install_requires=['opentracing>=1.2.2', 'grpcio>=1.8.1', 'six>=1.10'],


Interceptors exists within grpc exactly since 1.8.1?

I checked it's 1.8.0. And changed the requirement.

carlosalberto · 2019-01-11T07:22:18Z

+    def test_constructor_default(self):
+        interceptor = client_interceptor.OpenTracingClientInterceptor(Tracer(), True)
+        self.assertIsNotNone(interceptor._tracer)
+        self.assertTrue(interceptor._log_payloads)


Think there are other ways to test more into detail the new code?

If you don't have more time slots to spend on this, adding a TODO list for the tests or adding a note somewhere related to this would help ;)

carlosalberto · 2019-01-11T07:26:49Z

-from grpc_opentracing import open_tracing_client_interceptor
-from grpc_opentracing.grpcext import intercept_channel
+import grpc
+from grpc_opentracing.grpc_interceptor import client_interceptor


Trying to stay as compatible as before, I'd suggest exposing this new client_interceptor with the same name we had for the old one, i.e.

# __init__.py from grpc_interceptor import client_interceptor as open_tracing_ client_interceptor # old name, kept even if as deprecated from grpc_interceptor import client_interceptor # the actual name of the new object

Oh, and the same would go for server_interceptor ;)

carlosalberto · 2019-01-11T07:30:24Z

Hey @zhyon404 thanks for crafting all these changes! Definitely nice to finally use the official interceptor layer ;)

I left a few comments for you - let me know your thoughts. If you are busy with other stuff let me know, so I can help with this.

(And sorry for the delayed feedback!)

zhyon404 · 2019-01-21T04:13:30Z

Hey @carlosalberto, I added some changes according to your advice, I'm kind busy in next week, so it would be great if can help.

corleyma · 2019-08-01T20:59:09Z

@zhyon404 @carlosalberto Any plans to push this PR over the line? Is there anything I can do to help?

inquire · 2019-12-03T10:36:47Z

Is there anything in here that is not ok? This will make the code and unit tests for interceptor servers so much easier :D

electrofelix · 2020-09-24T15:21:47Z

+            scope.set_active_span(current_span)
+            scope.end_span()


this doesn't quite work they way it might be expected. I believe you need to pass the original span from before when you create a new one at https://github.com/opentracing-contrib/python-grpc/pull/14/files#diff-4ecafa65ccad14bb230575320502f261R57

The reason for this is that otherwise if there are further gRPC calls issued by the calling code after this point before it explicitly creates a new span each of these calls will appear as though the previous gRPC was it's parent.

e.g.

- start tracer span | \--- client grpc call 1 |--- server grpc function 1 | \--- ... other tracing calls in server \--- client grpc call 2 |--- server grpc function 2 | \--- ... other tracing calls in server \--- client grpc call 3 \--- server grpc function 3 \--- ... other tracing calls in server

instead of

- start tracer span | |--- client grpc call 1 | \--- server grpc function 1 | \--- ... other tracing calls in server |--- client grpc call 2 | \--- server grpc function 2 | \--- ... other tracing calls in server \--- client grpc call 3 \--- server grpc function 3 \--- ... other tracing calls in server

So the function should take two arguments def _callback(self, current_span, original_span): and then do current_span.finish() followed by scope.set_active_span(original_span). Since you want to finish the current span, but when exiting the grpc interceptor back to the program code the active span should be set back to what it was before the interceptor was called.

Add Implementation of the open-tracing interceptor using grpc interce…

b4bdc7c

…ptor. (opentracing-contrib#13)

zhyon404 force-pushed the master branch 2 times, most recently from f6d6696 to b4bdc7c Compare October 2, 2018 07:09

zhyon404 added 4 commits October 23, 2018 12:44

remove old implementations.

c63dde6

add example.

f6e6d85

Merge branch 'master' of https://github.com/opentracing-contrib/pytho…

2e82106

…n-grpc # Conflicts: # README.md

grpcio>=1.8.1

0893c8d

carlosalberto self-requested a review October 31, 2018 00:04

zhyon404 added 2 commits November 26, 2018 23:36

add ci back

165d306

fix ci

22cf5e2

carlosalberto reviewed Jan 11, 2019

View reviewed changes

carlosalberto mentioned this pull request Jan 12, 2019

plan to support official gRPC interceptors? #13

Open

zhyon404 and others added 4 commits January 21, 2019 11:36

Update config.yml

30ed3cb

Update README.md

300d7c0

Change minimal requirement of grpcio to 1.8.0

a6a3085

Use old name of interceptor

a12b8ad

Use old name of interceptor

d56d10c

Use old name of interceptor

c517c49

psiemens mentioned this pull request Apr 5, 2019

Ongoing maintenance of this repo #21

Open

william-plano-oxb mentioned this pull request Oct 23, 2019

add wrapper for grpc.Server.wait_for_termination #25

Open

tomnz mentioned this pull request Dec 11, 2019

Merge implementation of the open-tracing interceptor using gRPC interceptor oscarhealth/python-grpc#1

Merged

electrofelix reviewed Sep 24, 2020

View reviewed changes

Conversation

zhyon404 commented Sep 30, 2018

Uh oh!

rnburn commented Oct 1, 2018

Uh oh!

zhyon404 commented Oct 2, 2018

Uh oh!

rnburn commented Oct 4, 2018

Uh oh!

wenbochang commented Oct 15, 2018

Uh oh!

zhyon404 commented Oct 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosalberto Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosalberto commented Jan 11, 2019

Uh oh!

zhyon404 commented Jan 21, 2019

Uh oh!

corleyma commented Aug 1, 2019

Uh oh!

inquire commented Dec 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

carlosalberto Jan 11, 2019 •

edited

Loading