Better error handling in context managers for Span/Scope. #101

carlosalberto · 2018-08-14T01:53:54Z

Would address #72

carlosalberto · 2018-08-14T01:54:39Z

Probably the 'important' bit here is that we would also be setting the error tag/log for Span from Scope ;)

yurishkuro · 2018-08-14T16:28:32Z

opentracing/span.py

+        self.set_tag(tags.ERROR, True)
+        self.log_kv({
+            'event': tags.ERROR,
+            'error.object': exc_type,


this duplicates the previous log_kv, would it not make sense to combine?

Personally I don't have any preference there - so we can definitely put all that info in a single place ;) Will update.

I believe OT spec has the field name for the stacktrace, which takes care of python.exception.tb. I don't know what python.exception.val is about.

My preference is to converge to OT standard log fields instead of these custom python strings (which predate official OT fields). But it will be a breaking change, in some way. Worth checking with other tracing vendors if they have explicit dependency on these strings in the backend (Jaeger doesn't).

Oh, definitely. I had wondered about it too (that being said, it looks logging the error through error.object should be enough for us).

Will double check and ask away ;)

carlosalberto · 2018-08-15T15:24:47Z

Hey @palazzem @pglombardo

Wondering if any of your backend sides consume these custom python.exception.* logs? If not we will replace them with the standard ones ('error' as tag and 'error.object' as a log).

PS - Jaeger nor LightStep consume this field on the backend ;)

pglombardo · 2018-09-11T08:42:19Z

Hi @carlosalberto - sorry for the late reply (vacation). We don't have specific processing for the python.exception.* logs at Instana. No problem here on switching to the standard tags.

carlosalberto · 2018-10-08T13:50:58Z

Hey @yurishkuro think this could go finally in? Let me know ;)

yurishkuro · 2018-10-08T15:23:08Z

didn't we say we want to dedupe the logging?

carlosalberto · 2018-10-08T15:51:38Z

Oh my bad, thought I had already committed that :/ Will update it

carlosalberto · 2018-10-10T13:40:00Z

@yurishkuro Done (removed the custom fields). Let me know ;)

yurishkuro · 2018-10-14T20:56:31Z

opentracing/span.py

+        self.set_tag(tags.ERROR, True)
+        self.log_kv({
+            logs.EVENT: tags.ERROR,
+            logs.ERROR_OBJECT: exc_type,


I don't have a good understanding of Python exceptions, but this seems odd. Wouldn't this make more sense?

error.kind = exc_type

error.object = exc_val (??)

stack = exc_tb

Oh, I should win a prize for being so stupidly careless here, as it should have been exc_val - and also, I was trying to use the advised behavior from https://github.com/opentracing/specification/blob/master/semantic_conventions.md so we it should be enough to have the error object.

Updated ;)

can the tracer implementation still extract the stack trace if you only pass exc_val? What's the reason that Python splits those three?

Oh, good point - so, no, it's not possible in Python 2 at least. My guess was to go and append it as another log field, but I also see we don't have any stack field, as the Specification declares (this also happens in Java).

In any case, I think I will go ahead and add such traceback anyway. Let me know - the log field name can be added later on ;)

yurishkuro · 2018-10-16T15:26:18Z

opentracing/span.py

+        self.log_kv({
+            logs.EVENT: tags.ERROR,
+            logs.ERROR_OBJECT: exc_val,
+            'stack': exc_tb,


why not add stack constant as well? it's in the spec.

what about error.type tag, wouldn't it make sense to be set to exc_type?

carlosalberto · 2018-10-17T13:35:56Z

@yurishkuro Oh my, had forgotten we already had merged the opentracing.logs PR (and thus didn't want to mix them up).

So error.kind can be extrapolated from the exc_val but I'm appending it anymore - I'm also exposing an user-friendly traceback of the error (exactly the one you'd get running code). exc_tb is exposed here, but it's basically exposed for slightly more advanced usage, so we fallback to traceback.format_exc().

yurishkuro · 2018-10-17T16:16:35Z

opentracing/span.py

+            logs.ERROR_OBJECT: exc_val,
+            logs.ERROR_KIND: exc_type.__name__,
+            logs.MESSAGE: str(exc_val),
+            logs.STACK: traceback.format_exc(),


I have a slight preference to passing all 3 values above as is, without any additional work. If implementation wants to call .format_exc(), it can do so, but doing it explicitly in the API introduces a lot of unavoidable overhead, especially when the trace is not sampled.

In that case, we might need to use different log names, as per the spec, strings are expected. What's your take on this? Or simply put the objects there directly, anyway? (Personally I have mixed feelings about this, but I'd guess the latter option would be better ;) )

I think the spec is wrong about that. Forcing to-string prematurely is the biggest no-no of monitoring APIs. If we take Jaeger tracer, it will do to-string anyway, but only if the trace is actually sampled and the conversion is necessary. In some cases to-string is not even appropriate, e.g. if a tracing backend supports stack traces via proper structure, then I would much rather get the language-specific date structure and pack it more efficiently than to-string.

All right, lets do that. I just updated the PR ;)

carlosalberto · 2018-10-19T14:03:32Z

Merged ;) Thanks for the feedback!

…2.1.0 specifically, opentracing/opentracing-python#101

…2.1.0 (#471) specifically, opentracing/opentracing-python#101 closes #471

yurishkuro reviewed Aug 14, 2018

View reviewed changes

carlosalberto added 2 commits October 10, 2018 15:32

Better error handling in context managers for Span/Scope.

af84390

Drop the custom fields when reporting errors.

8b22204

carlosalberto force-pushed the error_tag_on_exc branch from 7a1d07d to 8b22204 Compare October 10, 2018 13:37

yurishkuro reviewed Oct 14, 2018

View reviewed changes

Use the proper exception object when logging errors.

4dbd65a

carlosalberto force-pushed the error_tag_on_exc branch from aeeb51d to 4dbd65a Compare October 16, 2018 14:30

Include the traceback object in the logs.

bed2514

yurishkuro reviewed Oct 16, 2018

View reviewed changes

Append all the error fields to the logs.

2d212c5

carlosalberto force-pushed the error_tag_on_exc branch from 4d2e54e to 2d212c5 Compare October 17, 2018 13:34

yurishkuro reviewed Oct 17, 2018

View reviewed changes

Pass the actual exception values instead of strings.

22a399c

yurishkuro approved these changes Oct 17, 2018

View reviewed changes

carlosalberto merged commit 5dda368 into opentracing:master Oct 19, 2018

carlosalberto mentioned this pull request Oct 23, 2018

Finishing Span on close #107

Open

carlosalberto mentioned this pull request Jan 8, 2019

Span does not set error tag if __exit__ is called with an exception #72

Closed

beniwohli added a commit to beniwohli/apm-agent-python that referenced this pull request Apr 29, 2019

adapt opentracing bridge to some minor changes in opentracing-python …

7b419d7

…2.1.0 specifically, opentracing/opentracing-python#101

beniwohli mentioned this pull request Apr 29, 2019

adapt opentracing bridge to some minor changes in opentracing-python 2.1.0 elastic/apm-agent-python#471

Closed

beniwohli added a commit to elastic/apm-agent-python that referenced this pull request Apr 29, 2019

adapt opentracing bridge to some minor changes in opentracing-python …

eac6ff8

…2.1.0 (#471) specifically, opentracing/opentracing-python#101 closes #471

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error handling in context managers for Span/Scope. #101

Better error handling in context managers for Span/Scope. #101

carlosalberto commented Aug 14, 2018

carlosalberto commented Aug 14, 2018

yurishkuro Aug 14, 2018

carlosalberto Aug 14, 2018 •

edited

Loading

yurishkuro Aug 14, 2018

carlosalberto Aug 14, 2018

carlosalberto commented Aug 15, 2018

pglombardo commented Sep 11, 2018

carlosalberto commented Oct 8, 2018

yurishkuro commented Oct 8, 2018

carlosalberto commented Oct 8, 2018

carlosalberto commented Oct 10, 2018

yurishkuro Oct 14, 2018

carlosalberto Oct 15, 2018 •

edited

Loading

yurishkuro Oct 15, 2018

carlosalberto Oct 16, 2018

yurishkuro Oct 16, 2018

carlosalberto commented Oct 17, 2018

yurishkuro Oct 17, 2018

carlosalberto Oct 17, 2018

yurishkuro Oct 17, 2018

carlosalberto Oct 17, 2018

carlosalberto commented Oct 19, 2018

Better error handling in context managers for Span/Scope. #101

Better error handling in context managers for Span/Scope. #101

Conversation

carlosalberto commented Aug 14, 2018

carlosalberto commented Aug 14, 2018

Choose a reason for hiding this comment

carlosalberto Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Aug 15, 2018

pglombardo commented Sep 11, 2018

carlosalberto commented Oct 8, 2018

yurishkuro commented Oct 8, 2018

carlosalberto commented Oct 8, 2018

carlosalberto commented Oct 10, 2018

Choose a reason for hiding this comment

carlosalberto Oct 15, 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

carlosalberto commented Oct 17, 2018

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

carlosalberto commented Oct 19, 2018

carlosalberto Aug 14, 2018 •

edited

Loading

carlosalberto Oct 15, 2018 •

edited

Loading