-
Notifications
You must be signed in to change notification settings - Fork 599
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.
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
gRPC streaming bugfix #260
gRPC streaming bugfix #260
Conversation
Is there anything you need me to do to help the process along here? This gRPC part of this is not really usable as-is, and I'd like to help with other things, but I'm blocked by these PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM!
Add tests for unary_stream calls. Add tests for verifing child spans get picked up and are partof the same trace properly.
86a5608
to
4332e7f
Compare
So for the record, it's also very frustrating when the lint check passes when the PR is made but sometime before it's merged, the linter rules get updated, and now a significant amount of the code fails the checks. Is there an announcement of these changes somewhere I could subscribe to? |
# Handle streaming responses separately - we have to do this | ||
# to return a *new* generator or various upstream things | ||
# get confused, or we'll lose the consistent trace | ||
def _intercept_server_stream( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single-use function here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the issue is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have been Nit: single-use function here.
Having a function that is used only once has the disadvantage of making the reader make a mental copy of the arguments passed to it, having to look for the function code, mentally pasting the arguments, reading the function code and then going back to the starting point without any of the advantages of not having repeated code that a function provides.
Anyways, it is still a nitpick, you can leave it as is if you please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this must be a function, for the reason explained in a the comment.
|
||
except Exception as error: | ||
# pylint:disable=unidiomatic-typecheck | ||
if type(error) != Exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why is this necessary? record_exception
expects an Exception
. Even so, if this check was appropriate, wouldn't it be better to use isinstance
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - in the gRPC world bare Exception
instances are thrown by the system when you call grpc.abort()
, and some of those shouldn't be treated as actual errors, for example the HTTP 404 series responses which are returned to the client but not recorded as errors in a trace.
This allows us to record anything except those as uncaught errors, since it's really unlikely anything else would throw a bare exception itself. It's not ideal but there just isn't any other way to distinguish these at this point in the program flow.
Using isinstance()
would catch application exceptions here, which we want to raise normally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see. This is unusual code, but necessary because of a particularity of gRPC that the reader may not know about. I'll approve, but please add this explanation as a comment here. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same exact code is on line 263, with a comment describing why it was done this way. Happy to duplicate the comment here if that would be preferred.
self.assertEqual(parent_span.name, rpc_call) | ||
self.assertIs(parent_span.kind, trace.SpanKind.SERVER) | ||
|
||
# Check version and name in span's instrumentation info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: redundant comment
Description
Fix for a bug I introduced when rewriting the gRPC instrumentation where child spans weren't properly being tracked as part of the same trace. Also there weren't tests for streaming responses before, so I've added one, and several to verify child spans are handled properly in both unary and streaming responses.
Fixes #257
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added new tests:
test_create_two_spans
test_create_span_streaming
test_create_two_spans_streaming
Also verified manually via exporting spans to Jaeger as part of a test application based on the sample server code.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.