Remove debug flag; Allow passing tags to start methods; Add Span.add_tags() method #3
Conversation
- Allow passing tags to start methods - Add Span.add_tags() method
| @@ -135,7 +146,7 @@ def is_debug(self): | |||
| return self.trace_context.flags & DEBUG_FLAG == DEBUG_FLAG | |||
|
|
|||
| def __str__(self): | |||
| from .encoding import TraceContextEncoder | |||
| from .marshaling import TraceContextEncoder | |||
There was a problem hiding this comment.
from an offline discussion w/ @bensigelman I thought we were going with...
Proposal:
%s/Marshaler/Encoder/g (etc)
%s/Unmarshaler/Decoder/g (etc)
trace_context_(from|to)_(binary|text) in python, TraceContext(From|To)(Binary|Text) in golang
There was a problem hiding this comment.
yeah, I assume this is just a rebase/merge issue?
There was a problem hiding this comment.
oh -- right. this is the zipkin dir. did I miss a file rename, maybe?
There was a problem hiding this comment.
this was just to fix that was left after renaming. We should rename the .py file itself
|
non-blocking observation about the marshaling thing. otherwise LGTM |
Remove debug flag; Allow passing tags to start methods; Add Span.add_tags() method
| @@ -63,18 +62,17 @@ def __exit__(self, exc_type, exc_val, exc_tb): | |||
| 'tb': exc_tb}) | |||
| self.finish() | |||
|
|
|||
| def start_child(self, operation_name): | |||
| def start_child(self, operation_name, tags=None): | |||
There was a problem hiding this comment.
stating the obvious here, but there's both
child_span = parent_span.start_child("op").add_tags(tags)
and
child_span = parent_span.start_child("op", tags)
Since it's easier to add surface area than remove surface area, I'd vote for keeping the former (more general) and removing the latter.
There was a problem hiding this comment.
I think this might be the case where language differences may result in minor divergence in the APIs. Accepting optional tags would be ugly in Go, but elegant in Python since the arg is not required. There are also performance reasons to prefer the second form when available - one less function call, one less mutex lock, and possibly one less memory allocation for the dictionary.
We actually haven't talked about memory allocation, I had a comment somewhere that by passing a dictionary to some call, the caller gives up ownership of that dictionary. We can either emphasize safety by requiring tracer to always take copies of passed collections if it wants to store them, or emphasize performance by telling the caller that it must always give up ownership of collections.
There was a problem hiding this comment.
Booked opentracing/opentracing-python#4 to continue this particular discussion.
No description provided.