Added batch spans sending #52

dmitry-prokopchenkov · 2017-07-18T09:46:22Z

This is a pr for #6

Guys, I have a question. I didn't provide a backward compatibility in this pull request. After my changes in thrift encoding code this example from the docs won't work:

import requests

def http_transport(encoded_span):
    # The collector expects a thrift-encoded list of spans. Instead of
    # decoding and re-encoding the already thrift-encoded message, we can just
    # add header bytes that specify that what follows is a list of length 1.
    body = '\x0c\x00\x00\x00\x01' + encoded_span
    requests.post(
        'http://localhost:9411/api/v1/spans',
        data=body,
        headers={'Content-Type': 'application/x-thrift'},
    )

I think here we breach encapsulation of thrift encoding logic by adding '\x0c\x00\x00\x00\x01'. In the pull request the count of thrift objects is defined automatically and can be hided from client. So the modified version of this example would be:

import requests

def http_transport(encoded_span):
    requests.post(
        'http://localhost:9411/api/v1/spans',
        data=encoded_span,
        headers={'Content-Type': 'application/x-thrift'},
    )

I can provide a backward compatibility by adding a 'batch' flag to zipkin_span constructor, but I prefer not to do it without your feedback. Please advise.

coveralls · 2017-07-18T09:49:54Z

Coverage remained the same at 100.0% when pulling 6b2e7c2 on dmitry-prokopchenkov:dprokopchenkov-send-batches-of-spans into 4f53165 on Yelp:master.

coveralls · 2017-07-18T10:15:03Z

Coverage remained the same at 100.0% when pulling 85a9ebf on dmitry-prokopchenkov:dprokopchenkov-send-batches-of-spans into 4f53165 on Yelp:master.

bplotnick

Thanks so much for submitting this. You're right about the leaky abstraction of the encoding of the span!

This mostly looks good, but I'm concerned that if a client doesn't call flush, then you could potentially get queued spans that are never emitted. I believe zipkin-reporter-java handles this by using a sending thread with a timeout so that the max sending delay is bounded. I wonder if we can do the same here?

@kaisen We will need to adjust a few internal tools before using this anywhere to check for the type of the thrift message: https://github.com/openzipkin/zipkin/blob/release-1.28.1/zipkin-collector/kafka/src/main/java/zipkin/collector/kafka/KafkaStreamProcessor.java#L62

bplotnick · 2017-07-18T22:48:32Z

py_zipkin/thrift/__init__.py

@@ -4,7 +4,8 @@
 import struct

 import thriftpy
-from thriftpy.protocol.binary import TBinaryProtocol
+from thriftpy.protocol.binary import TBinaryProtocol, write_list_begin


We split imports into one per line. If you run pre-commit, it should fix this. I believe pre-commit should be run as part of tox.

bplotnick · 2017-07-18T22:49:16Z

py_zipkin/logging_helper.py

-    span = create_span(
+class ZipkinBatchSender(object):
+
+    MAX_PORTION_SIZE = 100


Can you make this configurable?

kaisen · 2017-07-22T00:04:16Z

@bplotnick if I'm reading the code right, it looks like we won't have to worry about spans not being sent because flush() is always called with the logging context manager exits. Unless you're thinking about another corner case?

Still reviewing this PR.

bplotnick · 2017-07-22T00:39:26Z

@kaisen My concern was not with log_spans, but rather if someone uses ZipkinBatchSender separately. Perhaps we can call flush in the ZipkinBatchSender destructor?

coveralls · 2017-07-24T13:43:17Z

Coverage remained the same at 100.0% when pulling bd6a025 on dmitry-prokopchenkov:dprokopchenkov-send-batches-of-spans into 4f53165 on Yelp:master.

dmitry-prokopchenkov · 2017-07-24T13:48:16Z

@bplotnick re: flush(). I don't think zipkin-reporter-java with its timeouts is suitable for our case, because here we encode all spans together in zipkin_span.stop(). But we can use ZipkinBatchSender as a context manager and do flush() at __exit__().

dmitry-prokopchenkov · 2017-07-25T12:57:06Z

Guys, looking forward to your feedback about the latest changes

kaisen

looking good!

kaisen · 2017-07-25T20:34:40Z

README.md

@@ -142,13 +142,9 @@ your Zipkin collector is running at localhost:9411.
 import requests

 def http_transport(encoded_span):


A comment describing what type/format encoded_span is expected to be would be useful.

kaisen · 2017-07-25T20:45:10Z

py_zipkin/logging_helper.py

+            timestamp_s,
+            duration_s,
+        )
+        self._add_span_to_queue(thrift_span)


i'm thinking we don't need a separate function just to add it to the queue. i'm ok with _add_span_to_queue's logic be inside add_span

kaisen · 2017-07-25T21:17:27Z

py_zipkin/logging_helper.py

-    message = thrift_obj_in_bytes(span)
-    transport_handler(message)
+    ):
+        if not self.transport_handler:


i believe this check is unnecessary. a logging context is only created if perform_logging is True, which is only True if (self.zipkin_attrs or self.sampling_rate is not None) which requires a check for self.transport_handler.

kaisen · 2017-07-25T21:18:24Z

py_zipkin/zipkin.py

@@ -126,6 +127,9 @@ def __init__(
        :param transport_handler: Callback function that takes a message parameter
            and handles logging it
        :type transport_handler: function
+        :param max_span_portion_size: Spans in a trace are sent in batches,


doesn't matter too much but i would prefer 'max_span_batch_size'. @bplotnick thoughts?

Yeah I like the idea of having "batch" somewhere in the name, considering the batch sender class is called ZipkinBatchSender

kaisen · 2017-07-25T21:18:57Z

tests/integration/zipkin_integration_test.py

@@ -1,5 +1,5 @@
 import pytest
-from thriftpy.protocol.binary import TBinaryProtocol
+from thriftpy.protocol.binary import TBinaryProtocol, read_list_begin


2 separate import lines for this

dmitry-prokopchenkov · 2017-07-26T14:56:57Z

@kaisen, @bplotnick Please verify my latest commit. I took into account all your comments.

coveralls · 2017-07-26T14:57:48Z

Coverage remained the same at 100.0% when pulling bdef103 on dmitry-prokopchenkov:dprokopchenkov-send-batches-of-spans into 4f53165 on Yelp:master.

coveralls · 2017-07-26T14:57:48Z

Coverage remained the same at 100.0% when pulling bdef103 on dmitry-prokopchenkov:dprokopchenkov-send-batches-of-spans into 4f53165 on Yelp:master.

dmitry-prokopchenkov · 2017-07-31T08:16:27Z

Guys?

kaisen · 2017-07-31T17:10:37Z

Sorry for the delay @dmitry-prokopchenkov. This looks good to me. @bplotnick ?

bplotnick · 2017-07-31T17:58:18Z

bplotnick · 2017-07-31T18:00:42Z

Merged. I'll release a 0.9.0 in a bit with these changes. Thanks so much for this work @dmitry-prokopchenkov!!

dmitry-prokopchenkov · 2017-08-02T09:49:41Z

@bplotnick thanks! I really need this version in pip to complete my current task) When are you going to release this?

bplotnick · 2017-08-02T15:48:09Z

@kaisen I released 0.9.0 yesterday, but it didn't get uploaded to the public pypi (for some reason, i thought we had this automated). Can you do this so @dmitry-prokopchenkov can use the release?

bplotnick · 2017-08-02T18:46:08Z

@dmitry-prokopchenkov v0.9.0 is now on pypi

dmitry-prokopchenkov · 2017-08-03T08:07:23Z

@bplotnick , @kaisen Thanks!

Added batch spans sending

6b2e7c2

Added batch spans sending - minor changes

85a9ebf

bplotnick suggested changes Jul 20, 2017

View reviewed changes

dmitry-prokopchenkov added 3 commits July 24, 2017 15:52

minor changes

5f8335d

minor changes

df25e8c

minor changes

bd6a025

kaisen reviewed Jul 25, 2017

View reviewed changes

minor changes

bdef103

kaisen approved these changes Jul 31, 2017

View reviewed changes

bplotnick approved these changes Jul 31, 2017

View reviewed changes

bplotnick merged commit 275789a into Yelp:master Jul 31, 2017

kaisen mentioned this pull request Aug 4, 2017

logging spans via kafka or HTTP appear to be done in one big batch, synchronously #55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added batch spans sending #52

Added batch spans sending #52

dmitry-prokopchenkov commented Jul 18, 2017 •

edited

Loading

coveralls commented Jul 18, 2017 •

edited

Loading

coveralls commented Jul 18, 2017 •

edited

Loading

bplotnick left a comment

bplotnick Jul 18, 2017

bplotnick Jul 18, 2017

kaisen commented Jul 22, 2017

bplotnick commented Jul 22, 2017

coveralls commented Jul 24, 2017 •

edited

Loading

dmitry-prokopchenkov commented Jul 24, 2017

dmitry-prokopchenkov commented Jul 25, 2017

kaisen left a comment

kaisen Jul 25, 2017

kaisen Jul 25, 2017

kaisen Jul 25, 2017

kaisen Jul 25, 2017

bplotnick Jul 25, 2017

kaisen Jul 25, 2017

dmitry-prokopchenkov commented Jul 26, 2017

coveralls commented Jul 26, 2017 •

edited

Loading

coveralls commented Jul 26, 2017

dmitry-prokopchenkov commented Jul 31, 2017

kaisen commented Jul 31, 2017

bplotnick commented Jul 31, 2017

bplotnick commented Jul 31, 2017

dmitry-prokopchenkov commented Aug 2, 2017

bplotnick commented Aug 2, 2017

bplotnick commented Aug 2, 2017

dmitry-prokopchenkov commented Aug 3, 2017

		@@ -142,13 +142,9 @@ your Zipkin collector is running at localhost:9411.
		import requests

		def http_transport(encoded_span):

Added batch spans sending #52

Added batch spans sending #52

Conversation

dmitry-prokopchenkov commented Jul 18, 2017 • edited Loading

coveralls commented Jul 18, 2017 • edited Loading

coveralls commented Jul 18, 2017 • edited Loading

bplotnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaisen commented Jul 22, 2017

bplotnick commented Jul 22, 2017

coveralls commented Jul 24, 2017 • edited Loading

dmitry-prokopchenkov commented Jul 24, 2017

dmitry-prokopchenkov commented Jul 25, 2017

kaisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-prokopchenkov commented Jul 26, 2017

coveralls commented Jul 26, 2017 • edited Loading

coveralls commented Jul 26, 2017

dmitry-prokopchenkov commented Jul 31, 2017

kaisen commented Jul 31, 2017

bplotnick commented Jul 31, 2017

bplotnick commented Jul 31, 2017

dmitry-prokopchenkov commented Aug 2, 2017

bplotnick commented Aug 2, 2017

bplotnick commented Aug 2, 2017

dmitry-prokopchenkov commented Aug 3, 2017

dmitry-prokopchenkov commented Jul 18, 2017 •

edited

Loading

coveralls commented Jul 18, 2017 •

edited

Loading

coveralls commented Jul 18, 2017 •

edited

Loading

coveralls commented Jul 24, 2017 •

edited

Loading

coveralls commented Jul 26, 2017 •

edited

Loading