Skip to content

Conversation

@jizhang27
Copy link
Contributor

@jizhang27 jizhang27 commented Jun 8, 2018

Context

From opentracing spec, error tag should be respected to "decide if and only if the application considers the operation represented by the Span to have failed". true if and only if the application considers the operation represented by the Span to have failed

Currently we are relying on those three tags instead: ['error.type', 'error.msg', 'error.stack'].
reference

Approach

Set error flag to 1 if error tag is true. Also, skip writing it to meta since the error flag (not the tag) is used to classify the error.

TODO

  • Squash the commits if necessary

Closes #139

Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

However, the statement "decide if and only if the application considers the operation represented by the Span to have failed" is incorrect.

The exact wording from the specification for the error tag is "true if and only if the application considers the operation represented by the Span to have failed". This simply means that the error tag must not be set to true if the Span hasn't failed.

Since the backend relies on the error field and not the error tag, it should only be used by users who want to use the OpenTracing API.

The following should happen in the formatter when the error tag is set to true:

  • Set the error field to 1 on the Span since this is the value that is actually used by the backend to classify errors.
  • Discard the error tag since it' redundant after setting the error field and has no added value at this point.

For other use cases the current behavior is correct.

@jizhang27
Copy link
Contributor Author

Thanks for the comments @rochdev!

please correct me if i get it wrong, what opentracing only specifies the value of error tag itself instead of the error field used by the backed.

In this case, will my first commit be good enough then?

@rochdev
Copy link
Member

rochdev commented Jun 8, 2018

Yes, the first commit would indeed be the correct way to solve this issue.

- following opentracing standard, error tag should be true if the span has failed
- discard the tag in meta since the backend relies on the error flag
@jizhang27 jizhang27 force-pushed the fail_span_if_and_only_if_error_tag_is_true branch from d4bd00d to 4328053 Compare June 8, 2018 19:15
@jizhang27 jizhang27 changed the title Fail span if and only if error tag is true Fail span if error tag is true Jun 8, 2018
@jizhang27
Copy link
Contributor Author

comments addressed with description updated.

Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will be in next release. Thanks @jizhang27!

@rochdev rochdev merged commit 6eada51 into DataDog:master Jun 11, 2018
@jizhang27 jizhang27 deleted the fail_span_if_and_only_if_error_tag_is_true branch June 12, 2018 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working community core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants