Added processing for client root spans #49
Conversation
There was a problem hiding this comment.
Thanks so much for submitting this! And sorry for taking so long to review it... Overall it looks really good. Just a few comments.
Additionally, could add a test case that checks that the decorator adds the correct root client span? So something like with zipkin_client_span(...): or with zipkin_span(...include=('client',)). I think this should fail currently due to the issue that I describe below
py_zipkin/zipkin.py
Outdated
| @@ -281,6 +281,7 @@ def start(self): | |||
| if not self.zipkin_attrs.is_sampled: | |||
| return self | |||
| endpoint = create_endpoint(self.port, self.service_name, self.host) | |||
| client_context = (self.include == {'client'}) | |||
There was a problem hiding this comment.
self.include can be any iterable (i suppose we should assert this), so i don't think this will work unless you cast it to a set. So you should be able to do set(self.include) == {'client'}
Also you don't need the parentheses
There was a problem hiding this comment.
thanks!
py_zipkin/logging_helper.py
Outdated
|
|
||
| The trace can represent | ||
| 1) server and contains root server span and child client spans (optionally) | ||
| 2) client and contains root client span only |
There was a problem hiding this comment.
I think this new comment is not quite correct. The original comment was saying basically that the root span is a server span, but the local child spans can be client and/or server. In fact, our "local" spans use both cs/cr and ss/sr annotations.
You can still have this with a client root span. Imagine a application that will download some file, and also has some local method for validation that we instrumented. We might have
cs: download_file
cs/sr/ss/sr: validate_parameters_locally
cs: fetch_file_from_server
cr: fetch_file_from_server
cr: download_file
You could even imagine child server spans in there if somewhere in the middle we check some queue for new data.
All of this is pretty unfamiliar, so i'm open to any other opinions on this matter.
There was a problem hiding this comment.
totally agree with you, will change the comment
py_zipkin/logging_helper.py
Outdated
| @@ -97,6 +101,7 @@ def log_spans(self): | |||
| ) | |||
|
|
|||
| # Collect, annotate, and log client spans from the logging handler | |||
| # In case of root client span client_spans list is empty | |||
There was a problem hiding this comment.
same comment as above, I don't think that this is quite correct
There was a problem hiding this comment.
I removed my comment. May be it would be better to rename client_spans (self.log_handler.client_spans) to child_spans?
There was a problem hiding this comment.
I'd be totally cool with that if you'd like to tack that on to this PR. Otherwise, I can do that as a followup task.
There was a problem hiding this comment.
The second option, I can do that, but I would like to do it separately.
|
looks good to me! @kaisen can you take a quick look and see if anything pops out to you? |
|
@bplotnick I am not good familiar with github pull request workflow. Should I close this pull request? Or at first wait for @kaisen's answer? |
|
@dmitry-prokopchenkov once @kaisen looks at it (i just poked him), assuming he has no objection, i will go ahead and merge your PR |
|
nice! looks good |
|
@dmitry-prokopchenkov this has been merged. Thanks so much for this! |
|
thank you, guys! |
This is pull request for #35