Set error tag to false when string value is false#97
Set error tag to false when string value is false#97mmenindezsegura merged 3 commits intoExpediaDotCom:masterfrom
Conversation
|
|
||
| private static List<Tag> fromZipkinTag(String key, String value) { | ||
| if ("error".equalsIgnoreCase(key)) { | ||
| if ("error".equalsIgnoreCase(key) && !"false".equalsIgnoreCase(value)) { |
There was a problem hiding this comment.
@absrivastava @kapilra Would like your input on this one, change seems pretty straightforward, however I'm not sure if not setting this tag when it's false could have more implications that I'm not aware.
There was a problem hiding this comment.
To me it shouldn't matter, if it's false ... I don't see any problem with this, Just want to double check.
There was a problem hiding this comment.
As far as I remember back in the DT workshop in Gurgaon this was something Haystack supported to have feature parity with zipkin in terms of error cc @worldtiki
There was a problem hiding this comment.
Maybe a better approach would be an else statement where it sets the errorTag explicitly to false when it's handling an error tag with a false value instead of skipping altogether. That way we're still transforming the tag from string to boolean but not setting it to true all the time.
There was a problem hiding this comment.
@eocantu is this something that you are adding manually or is the instrumentation lib that you are using adding this tag?
I'm asking because the error tag should contain a string with an error message and it's the existence of the tag that determines if the span contains errors or not, and not the value of the tag.
Ie, error=false means that there was an error and that the error message is "false".
Even though this is semantically incorrect, I can't think of anyone who would use the word "false" as an error message. Given that the two models (Haystack and Zipkin) are similar but not exactly alike I understand the reason for this change but I think it would be useful to know if this tag was added manually or not.
Regarding explicitly setting it to false when there are no errors, that's basically a no-op statement and will just add more information to transmit.
There was a problem hiding this comment.
@worldtiki Per the opentracing API, the error tag is a boolean. And if I understand correctly, technically you can set it to false and it would still be valid (regardless of whether you transmit more information or not).
Our instrumentation uses Jaeger with a reporter that uses the ThriftSpanConverter and all tags are simply converted to strings. I also tested the ZipkinV2Reporter which uses the V2SpanConverter and you also end up with an error string tag with the value of false
eocantu
changed the title
4 participants
This prevents setting the error tag to true when the zipkin error tag value is "false"