Zipkin firehose #66
Zipkin firehose #66
Conversation
py_zipkin/logging_helper.py
Outdated
| if self.firehose_handler: | ||
| self._log_spans_with_span_sender( | ||
| ZipkinBatchSender(self.firehose_handler, | ||
| self.max_span_batch_size) |
There was a problem hiding this comment.
the 2 max_batch_size don't have to be the same as the zipkin api and collectors don't have the same MTU limitations we have for firehose
There was a problem hiding this comment.
I added this as a FIXME for this branch, since I am not sure how we want to handle this, so for now we can assume both max sizes are the same.
py_zipkin/zipkin.py
Outdated
| @@ -113,7 +113,8 @@ def __init__( | |||
| add_logging_annotation=False, | |||
| report_root_timestamp=False, | |||
| use_128bit_trace_id=False, | |||
| host=None | |||
| host=None, | |||
| firehose_handler=None | |||
There was a problem hiding this comment.
Do we want people to re-implement the firehose handler then? I thought we had decided to implement it in py_zipkin directly. In that case this could just be a boolean flag.
There was a problem hiding this comment.
Personally, I think we should keep it as a reference to a function. I'd rather see a default implementation that people can import and pass as an argument. That way, users can define their own handlers.
There was a problem hiding this comment.
I will add a reference implementation here once we have a design doc and spec for the local firehose daemon. But, at least for now, I agree with @msindwan that we can leave it up to the user.
| @@ -285,7 +288,7 @@ def start(self): | |||
|
|
|||
There was a problem hiding this comment.
I think you also need to check self.firehose_handler in line 246 (self.perform_logging = bool(...)).
github doesn't let me add a comment there since it's far from your changes :(
There was a problem hiding this comment.
Good call, though I'm not really sure what this code path would even do. In what case would you have no sample rate, no passed in zipkin attrs, and no ongoing trace? Maybe if you called some function that is usually an "inner span"? I dunno. Anyway, I added a test for it and fixed the issue.
|
Thanks for sharing this!
Link didnt work: openzipkin/zipkin#1869 please
link any other details you might have
So assume if doc'd this would be like..
When present, firehose_handler records spans for 100% of requests (ignoring
sample_rate). Implementations might aggregate span metrics or fill a time
limited trace depot. Take care to implement asynchronously to avoid excess
overhead on all requests.
(Probably text isnt awesome)
|
py_zipkin/zipkin.py
Outdated
| @@ -260,7 +264,7 @@ def start(self): | |||
| ) | |||
|
|
|||
| if not self.zipkin_attrs: | |||
| # This span is inside the context of an existing trace | |||
| # Is this span is inside the context of an existing trace? | |||
There was a problem hiding this comment.
nit: "Is this span is inside.."
| # with sample rate of 0 | ||
| report_root_timestamp = True | ||
| self.zipkin_attrs = create_attrs_for_span( | ||
| sample_rate=0.0, |
There was a problem hiding this comment.
I'm a little confused here - wouldn't we want a 100% sampling rate on the span? Or is that not how it works?
There was a problem hiding this comment.
Sorry I forgot to respond to you. As far as the attrs is concerned, the sample rate is 0%. The firehose is separate from the sample rate.
| retention. It works in tandem with normal operation, however there may | ||
| be additional overhead. In order to use this, you add a | ||
| `firehose_handler` just like you add a `transport_handler`. | ||
|
|
There was a problem hiding this comment.
If someone wanted to enable only firehose mode and disable the normal transport, they could simply set the sampling rate to 0% and provide the firehose_handler, correct? Could be worth mentioning that here.
There was a problem hiding this comment.
I feel like this is implied in "regardless of sampling rate". I'm gonna merge as-is. We'll need to add additional docs about firehose anyway (e.g. example handler and proxy daemon) so if this still isn't clear I'll fix it up later
This is a quick and dirty implementation of the "firehose mode" (openzipkin/zipkin#1869). The full specification for firehose mode is to come, but the basic idea is that there is a second handler that emits spans regardless of sampling decision.
@msindwan @drolando