Adopt Sender.append() and flush() #1

rmfitzpatrick · 2018-07-12T20:36:47Z

Implements the Sender.append() and Sender.flush() methods requested in jaegertracing#186 (review) in an attempt to get those changes sourced.

Signed-off-by: Ryan Fitzpatrick rmfitzpatrick@signalfx.com

rmfitzpatrick · 2018-07-12T20:41:32Z

@ProvoK, if you're interested in continuing with these changes, do you mind rebasing your branch to update the dependencies for successful CI runs?

ProvoK · 2018-07-13T15:43:18Z

@rmfitzpatrick Hello there,

thanks for continuing the work! Yes I'm interested, I was a lot busy in the last period, that's why I was not working on it.

I'll update my branch and review your change asap 😉

ProvoK · 2018-07-13T15:48:16Z

@rmfitzpatrick I've fetched upstream master and merged into my branch. This is all I can do till Monday, sorry!

codecov-io · 2018-07-13T15:59:42Z

Codecov Report

Merging #1 into send_spans_over_http will increase coverage by 0.36%.
The diff coverage is 100%.

@@                   Coverage Diff                    @@
##           send_spans_over_http       #1      +/-   ##
========================================================
+ Coverage                 94.86%   95.23%   +0.36%     
========================================================
  Files                        26       26              
  Lines                      1812     1824      +12     
  Branches                    227      227              
========================================================
+ Hits                       1719     1737      +18     
+ Misses                       59       55       -4     
+ Partials                     34       32       -2

Impacted Files	Coverage Δ
jaeger_client/senders.py	`98.43% <100%> (+4.68%)`	⬆️
jaeger_client/reporter.py	`95.86% <100%> (+2.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 435b532...073d2ba. Read the comment docs.

Signed-off-by: Ryan Fitzpatrick <rmfitzpatrick@signalfx.com>

ProvoK · 2018-07-16T19:45:59Z

jaeger_client/reporter.py

@@ -168,14 +160,15 @@ def _report_span_from_ioloop(self, span):

    @tornado.gen.coroutine
    def _consume_queue(self):
-        spans = []
+        spans_appended = 0


Keeping the count of an internal state of the sender outside of it doesn't seem to me like a good idea.
It's better that the sender expose a property like spans_count which returns the real and actual count of its spans.
With that, you can easily get rid of updating this spans_appended variable (line 183, 187), with a more secure and less error prone behaviour.

Great idea, updated.

ProvoK · 2018-07-16T19:47:23Z

jaeger_client/senders.py


+        raise tornado.gen.Return((num_spans, exc, error_msg))


I'm not very skilled with tornado: is there a valid reason to handle exceptions in this manner?

This is a special exception that allows coroutines to return values in older versions of python: http://www.tornadoweb.org/en/stable/gen.html#tornado.gen.Return. It's necessary to get the exception info from the send generator.

Ok, I got it. Still I'm not sure this is necessary.

The only return value used is num_spans, which is everytime equal to sender.span_count with the actual implementation.

This means that you can simply save a variable here (something like num_spans = self._sender.span_count) and then use it later for reporting the number of success/error spans.

Everything else (exceptions and theirs messages) can be handled normally, I suppose.
@rmfitzpatrick

Of course, makes more sense to fully adopt span_count and I've updated. However, I did retain the Sender._flush() generated exception info so that transport-specific exception handling is possible in Sender and its subclasses, without having to maintain a complete, sender-aware try/except block in Reporter._consume_queue(). Does that work for you, @ProvoK?

ProvoK · 2018-07-16T19:48:18Z

jaeger_client/senders.py

    def send(self, batch):
+        """
+        Send batch of spans out via thrift transport. Any exceptions thrown


This docstring does not belongs to this method

Good call. Updated.

ProvoK · 2018-07-16T19:49:32Z

jaeger_client/senders.py

-    def __init__(self, host, port, io_loop=None):
-        self.host = host
-        self.port = port
+    def __init__(self, io_loop=None):


host and port are likely to be needed in every sender implementations. I would keep them in the base class

In my early HTTPSender implementation I found only a url is necessary, and constructing one via host, port, and trace endpoint seems overkill (other jaeger clients use JAEGER_ENDPOINT env var to specify the collector location).

Ok, let's see if jaeger maintainers are ok with this interface

Do you mean before merging into your PR branch?

Nono, I mean after merging in my PR branch. The only one remaining issue is the one related to tornado.gen.Return, I replied to you there yesterday

Sorry, I missed that comment.

rmfitzpatrick · 2018-07-17T14:31:43Z

@ProvoK, thanks for the feedback, which I believe I've addressed as requested with the exception of #1 (comment) to match the logic of other clients. Let me know if you still request that change and I'll be happy to do so, or if you have any other notes.

ProvoK · 2018-07-19T18:53:06Z

jaeger_client/reporter.py

+
+            if self._sender.span_count:
+                num_spans = self._sender.span_count
+                exc, error_msg = yield self._sender.flush()


I don't get the benefit of that, honestly. What you are doing now is a try ...except Exception ... in the Sender.flush(), returning the exception plus a message. Which is the same as an raise AnyException("message")

In the Reporter you just check if any kind of exc is raised, and the use the message returned.

This is the very same of:

try: yield self._sender.flush() except Exception as exc: self.metrics.reporter_failure(num_spans) self.error_reporter.error(error_msg, exc) else: self.metrics.reporter_success(num_spans)

but this is the most common and standard way to deal with exceptions, in Python.

The point is to keep the code "idiomatic".
In other languages, as Go, the standard is to have the pattern you are doing: multiple returns, one for the error/ok-boolean and the other for the "real" value.

Where does error_msg come from in this excerpt? Providing error_reporter.error() with str(exc) and exc loses the information provided by the initial local-agent submission failure: https://github.com/jaegertracing/jaeger-client-python/blob/master/jaeger_client/reporter.py#L201

This pattern could be repeated with all manner of clients in the future, who idiomatically catch their transport specific exceptions and provide a relevant exception message.

I was writing in this little github editor so I lost the context

try: yield self._sender.flush() except Exception as exc: self.metrics.reporter_failure(num_spans) self.error_reporter.error("Failed to submit traces to jaeger: {}".format(exc), exc) else: self.metrics.reporter_success(num_spans)

You can still catch below, in the sender, the specific error and provide a relevant message, carried with the exception itself.

Providing a custom message in send() seems effective. Are you comfortable with using something like six.reraise to add a custom value with the traceback from sys.exc_info()?

I would not add six as dependency just for that. And probably jaeger maitaners wouldn't too

It already is one via thrift.

edit: Not to speak in circles, but I'm not aware of a version agnostic way to override an exception value without making Reporter sender detail aware or less informative otherwise.

Oh, didn't know it was included in thrift. Then I suppose it's allright to use it

ProvoK · 2018-07-19T18:54:33Z

jaeger_client/senders.py

+        try:
+            yield self.send(batch)
+        except Exception as e:
+            exc = e


Here you can just raise e("my message") from e and it's done! (alongwith the previous comment modifications)

The from e part preserve the exception stacktrace

from e is not supported in python 2.

Yeah, you're right. I'm too used to python3

ProvoK · 2018-07-19T18:59:37Z

jaeger_client/senders.py

@@ -81,6 +134,24 @@ def _create_local_agent_channel(self, io_loop):
            io_loop=io_loop
        )

+    @tornado.gen.coroutine
+    def _flush(self, spans, process):
+        exc = None


Again here, you can save a lot of lines of code

rmfitzpatrick · 2018-07-19T21:47:51Z

edit: User error with a stale test.

I really appreciate your thorough feedback and believe I've addressed it all, @ProvoK.

Also handle exceptions normally over marshalling as future result. Signed-off-by: Ryan Fitzpatrick <rmfitzpatrick@signalfx.com>

ProvoK · 2018-07-22T15:38:11Z

Thanks @rmfitzpatrick for your work, I'm going to merge this, and I'll quote in in the original PR.

rmfitzpatrick force-pushed the send_spans_over_http branch 2 times, most recently from 99b5bd5 to d43a89e Compare July 12, 2018 21:18

ProvoK force-pushed the send_spans_over_http branch from 8f070d1 to 435b532 Compare July 13, 2018 15:46

rmfitzpatrick force-pushed the send_spans_over_http branch from b795720 to ef41bed Compare July 13, 2018 15:51

Adopt Sender.append() and flush()

0ac58e8

Signed-off-by: Ryan Fitzpatrick <rmfitzpatrick@signalfx.com>

rmfitzpatrick force-pushed the send_spans_over_http branch from ef41bed to 0ac58e8 Compare July 16, 2018 16:07

ProvoK requested changes Jul 16, 2018

View reviewed changes

rmfitzpatrick force-pushed the send_spans_over_http branch from 0b9edef to 8708e1f Compare July 19, 2018 13:46

ProvoK requested changes Jul 19, 2018

View reviewed changes

rmfitzpatrick force-pushed the send_spans_over_http branch 2 times, most recently from 5e9d687 to 361b5f5 Compare July 19, 2018 20:45

rmfitzpatrick force-pushed the send_spans_over_http branch 3 times, most recently from b8a9656 to 29ead94 Compare July 19, 2018 22:24

Use Sender.span_count for initiating flush

073d2ba

Also handle exceptions normally over marshalling as future result. Signed-off-by: Ryan Fitzpatrick <rmfitzpatrick@signalfx.com>

rmfitzpatrick force-pushed the send_spans_over_http branch from 29ead94 to 073d2ba Compare July 20, 2018 13:56

ProvoK approved these changes Jul 22, 2018

View reviewed changes

ProvoK merged commit fbefb07 into ProvoK:send_spans_over_http Jul 22, 2018

rmfitzpatrick deleted the send_spans_over_http branch August 10, 2018 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt Sender.append() and flush() #1

Adopt Sender.append() and flush() #1

rmfitzpatrick commented Jul 12, 2018

rmfitzpatrick commented Jul 12, 2018

ProvoK commented Jul 13, 2018

ProvoK commented Jul 13, 2018 •

edited

Loading

codecov-io commented Jul 13, 2018 •

edited

Loading

ProvoK Jul 16, 2018

rmfitzpatrick Jul 17, 2018

ProvoK Jul 16, 2018

rmfitzpatrick Jul 17, 2018

ProvoK Jul 18, 2018 •

edited

Loading

rmfitzpatrick Jul 19, 2018 •

edited

Loading

ProvoK Jul 16, 2018

rmfitzpatrick Jul 17, 2018

ProvoK Jul 16, 2018

rmfitzpatrick Jul 17, 2018

ProvoK Jul 18, 2018

rmfitzpatrick Jul 18, 2018

ProvoK Jul 19, 2018

rmfitzpatrick Jul 19, 2018

rmfitzpatrick commented Jul 17, 2018

ProvoK Jul 19, 2018

rmfitzpatrick Jul 19, 2018

ProvoK Jul 19, 2018 •

edited

Loading

rmfitzpatrick Jul 19, 2018 •

edited

Loading

ProvoK Jul 19, 2018

rmfitzpatrick Jul 19, 2018 •

edited

Loading

ProvoK Jul 19, 2018

ProvoK Jul 19, 2018

rmfitzpatrick Jul 19, 2018

ProvoK Jul 19, 2018

ProvoK Jul 19, 2018

rmfitzpatrick commented Jul 19, 2018 •

edited

Loading

ProvoK commented Jul 22, 2018

Adopt Sender.append() and flush() #1

Adopt Sender.append() and flush() #1

Conversation

rmfitzpatrick commented Jul 12, 2018

rmfitzpatrick commented Jul 12, 2018

ProvoK commented Jul 13, 2018

ProvoK commented Jul 13, 2018 • edited Loading

codecov-io commented Jul 13, 2018 • edited Loading

Codecov Report

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 Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

rmfitzpatrick Jul 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmfitzpatrick commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProvoK Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

rmfitzpatrick Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmfitzpatrick Jul 19, 2018 • edited Loading

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

rmfitzpatrick commented Jul 19, 2018 • edited Loading

ProvoK commented Jul 22, 2018

ProvoK commented Jul 13, 2018 •

edited

Loading

codecov-io commented Jul 13, 2018 •

edited

Loading

ProvoK Jul 18, 2018 •

edited

Loading

rmfitzpatrick Jul 19, 2018 •

edited

Loading

ProvoK Jul 19, 2018 •

edited

Loading

rmfitzpatrick Jul 19, 2018 •

edited

Loading

rmfitzpatrick Jul 19, 2018 •

edited

Loading

rmfitzpatrick commented Jul 19, 2018 •

edited

Loading