Refactor for future span sending over http #176

ProvoK · 2018-05-20T19:05:39Z

Ideally you should not need to change in the crossdock code, because if you do it means your change is not backwards compatible.

I changed it because Reporter and RemoteControlledSampler init changes.

EDIT: reverted changes on RemoteControlledSampler and make io_loop parameter not required in Sender classes (LocalAgentSender handles io_loop=None). This will impact less actual existing codebase (and tests)

coveralls · 2018-05-20T19:59:07Z

Coverage increased (+0.08%) to 95.43% when pulling dfd6548 on ProvoK:send_spans_over_http into 9429e29 on jaegertracing:master.

ProvoK · 2018-05-21T08:59:35Z

jaeger_client/senders.py

+from jaeger_client.thrift_gen.agent import Agent
+
+
+DEFAULT_SAMPLING_PORT = 5778


This default should probably be moved to constants.
Actually is defined only in config (and here ofc)

black-adder

sorry for the delay, i'll do a full review soon

black-adder · 2018-05-25T04:17:20Z

crossdock/server/endtoend.py

@@ -65,8 +66,13 @@ def __init__(self):
        init_sampler = cfg.sampler
        channel = self.local_agent_sender

+        sender = UDPSender(
+            io_loop=channel.io_loop,
+            host=cfg.local_agent_reporting_host,


here use the host port from local_agent_sender:

jaeger-client-python/crossdock/server/endtoend.py

Line 105 in cb14ed8

host=os.getenv('AGENT_HOST', 'jaeger-agent'),

oh gawd it was the problem! I was really struggling.Thanks!

ProvoK · 2018-05-25T05:16:28Z

@black-adder no problem, I'll squash commits and fix sign-off missing before merging. Plus, I'm thinking I should test senders.py module in some way

ProvoK · 2018-05-25T06:58:36Z

jaeger_client/senders.py

+        :param self: instance of Config
+        """
+        logger.info('Initializing Jaeger Tracer with UDP reporter')
+        return LocalAgentSender(


As I said earlier here I would like to split responsibility between LocalAgentSender and LocalAgentHttp (which actually just perform an HTTP call for the sampling). I could do the separation in this PR, I removed in order to keep the changes small.
This would remove the need of the sampling_port argument

I did all but this one. This will touch a lot of things (Sampler and Config), so it's probably better do it in a separate PR

black-adder

Looks great, can we add tests to cover the new functions in jaeger_client/senders.py? Also, you're going to have to sign off on all your commits, you can do that by squashing all your commits and signing it and then doing a force push

black-adder · 2018-05-28T13:44:32Z

jaeger_client/senders.py

@@ -0,0 +1,90 @@
+# Copyright (c) 2016 Uber Technologies, Inc.


that should be 2018, looks like there's something wrong with the license script

black-adder · 2018-05-28T13:51:33Z

jaeger_client/senders.py

+        :param self: instance of Config
+        """
+        logger.info('Initializing Jaeger Tracer with UDP reporter')
+        return LocalAgentSender(


black-adder · 2018-05-28T13:53:50Z

jaeger_client/senders.py

+    def send(self, batch):
+        """
+        Send batch of spans out via thrift transport. Any exceptions thrown
+        will be caught above in the exception handler of _submit().


some of the comments need updating like here

ProvoK · 2018-05-28T20:54:01Z

Tomorrow I'm going to add some unit test for sender.py and fix a documentation line in the reporter class

`Reporter` now delegate span sending to new `Sender` classes . Signed-off-by: vitto <vittorio.camisa@gmail.com>

yurishkuro · 2018-05-30T03:06:20Z

jaeger_client/config.py

        reporter = Reporter(
-            channel=channel,
+            sender=sender,


@black-adder renaming the field is a breaking change. Are you sure this can be merged?

doh reverting

let's find a way to let this PR to get merged, but in backwards compatible way.

I did changes to make it backwards compatible. Should I open another PR?

edit: Still I don't like much the workaround I did. If you got better ideas I'm open

This reverts commit 51df6ab.

ProvoK changed the title ~~Refactor for future span sending over http~~ WIP: Refactor for future span sending over http May 20, 2018

ProvoK force-pushed the send_spans_over_http branch 4 times, most recently from f6eb07b to a4955e1 Compare May 20, 2018 19:42

ProvoK force-pushed the send_spans_over_http branch from 715e034 to 514a0d8 Compare May 21, 2018 08:57

ProvoK commented May 21, 2018

View reviewed changes

ProvoK force-pushed the send_spans_over_http branch 2 times, most recently from 1246b60 to 111e096 Compare May 21, 2018 09:04

black-adder reviewed May 25, 2018

View reviewed changes

ProvoK changed the title ~~WIP: Refactor for future span sending over http~~ Refactor for future span sending over http May 25, 2018

ProvoK commented May 25, 2018

View reviewed changes

black-adder reviewed May 28, 2018

View reviewed changes

ProvoK force-pushed the send_spans_over_http branch from 46ab59a to d004867 Compare May 28, 2018 20:48

Preliminary refactor for supporting spans over HTTP

dfd6548

`Reporter` now delegate span sending to new `Sender` classes . Signed-off-by: vitto <vittorio.camisa@gmail.com>

ProvoK force-pushed the send_spans_over_http branch from d004867 to dfd6548 Compare May 29, 2018 05:10

black-adder approved these changes May 29, 2018

View reviewed changes

black-adder merged commit 51df6ab into jaegertracing:master May 29, 2018

yurishkuro reviewed May 30, 2018

View reviewed changes

black-adder added a commit that referenced this pull request May 30, 2018

Revert "Preliminary refactor for supporting spans over HTTP (#176)"

d0dd4b2

This reverts commit 51df6ab.

This was referenced May 30, 2018

Revert "Refactor for future span sending over http" #183

Closed

Revert 'Refactor for future span sending over http' #184

Merged

ProvoK mentioned this pull request May 31, 2018

Preliminary refactor for supporting spans over HTTP #186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor for future span sending over http #176

Refactor for future span sending over http #176

ProvoK commented May 20, 2018 •

edited

Loading

coveralls commented May 20, 2018 •

edited

Loading

ProvoK May 21, 2018

black-adder left a comment

black-adder May 25, 2018

ProvoK May 25, 2018

ProvoK commented May 25, 2018

ProvoK May 25, 2018

black-adder May 28, 2018

ProvoK May 29, 2018

black-adder left a comment

black-adder May 28, 2018

black-adder May 28, 2018

black-adder May 28, 2018

ProvoK commented May 28, 2018

yurishkuro May 30, 2018

black-adder May 30, 2018

yurishkuro May 31, 2018

ProvoK May 31, 2018 •

edited

Loading

		from jaeger_client.thrift_gen.agent import Agent


		DEFAULT_SAMPLING_PORT = 5778

		@@ -0,0 +1,90 @@
		# Copyright (c) 2016 Uber Technologies, Inc.

Refactor for future span sending over http #176

Refactor for future span sending over http #176

Conversation

ProvoK commented May 20, 2018 • edited Loading

coveralls commented May 20, 2018 • edited Loading

Choose a reason for hiding this comment

black-adder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProvoK commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-adder 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

ProvoK commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProvoK May 31, 2018 • edited Loading

Choose a reason for hiding this comment

ProvoK commented May 20, 2018 •

edited

Loading

coveralls commented May 20, 2018 •

edited

Loading

ProvoK May 31, 2018 •

edited

Loading