Improve UX when multiple binary annoations exist in the same span, with the same key

[codefromthecrypt]

Why does the same binary annotation pop up twice? In zipkin, it is possible to have multiple hosts in the same span. Each of them can log the same binary annotation (tag). ex. http.status. When they match, this looks like a duplicate. When they don't match, this could be helpful (ex the client perceived an error even if the server didn't perceive one. Single-host spans by definition don't have this problem (since only one host ever logs anything in a span).

Here's an option @yurishkuro discussed about uncovering why it looks like a dupe. Basically, if we add a column for the origin of the value, it might make more sense. I've pasted some other thoughts of his below as well.

Another common scenario I've seen with same keys is the http.url, which is sent by both client and server, and the value may or may not be the same.

If we adopt the suggestion that (spanID, spanKind, emittingService) combination is unique (which I think implies ditching core annotations and moving the Endpoint on the Span itself), then seemingly identical binary annotations may still be distinguishable by the service that emits them.

[jcarres-mdsol]

we at least are careful to not send some stuff we know will be duplicated when the upstream system is also zipkin
https://github.com/openzipkin/zipkin-tracer/blob/master/lib/zipkin-tracer/rack/zipkin-tracer.rb#L55

[codefromthecrypt]

per @CodingFabian

we also see dupes. because we do not know if the dupes are guaranteed, we think the UI should filter dupes (our on ui does that)

[yurishkuro]

I think it's ok to filter dups, provided that high fidelity data is used for comparisons.

span id=1
   anno:
     sr, host=1.2.3.4
   bin_anno:
     x = y
span id=1
   anno:
     cs, host=5.6.7.8
   bin_anno:
     x = y

here the two identical binary annotations are actually sent by different servers, and may have different meaning. This correlation gets lost once the spans are merged in the UI into one.

[codefromthecrypt]

@yurishkuro do you have any particular use case where the key and value is the same for different hosts in the same span, and knowing that would have helped solve a latency issue?

[yurishkuro]

no

[codefromthecrypt]

One thing to bear in mind here is that I don't think we've ever raised here is some UI changes imply cementing in aspects of the zipkin data model, which aren't universally popular and may limit future model choices.

For example, the binary annotation data structure is not popular. There exists a sentiment that they could be simpler, ex a String->String map. If we formally support reporting the originating host of a binary annotations, this limits model options to those that intentionally allow repeats. Zipkin v2 could solve this, if it stays single-host spans. If it doesn't, and a String->String map was chosen, then changing the UI to report the origin of a tag wouldn't be supportable.

This situation is not new, by the way, just solving it hasn't proved popular. Even here, multiple folks are working around this as opposed to asking for this to be changed. Ex filtering duplicates, or they have adjusted their tracers to avoid the ambiguity. Personally, I'm happy about that, as the act of "choosing" on conflict for most fields, particularly span name, has proven high-maintenance code. Leaving that undefined for routine binary annotations is less code to maintain.

Since the time we introduced zipkin's http api, one could troubleshoot any issue like this by looking at the json for the trace. In fact, I noticed in a screen shot that Uber actually have "json" UI element, which could make navigation even easier. I'd recommend we sit on this and consider it for zipkin v2 vs investing further time into this topic, as rehashing this hurts the project by displacing time.

In the mean time, we might want to formalize a UI element to make getting to the raw json easier.

[abesto]

Slightly confused here. Both Annotation and BinaryAnnotation have an optional Endpoint field (going by https://github.com/openzipkin/zipkin/blob/master/zipkin-thrift/src/main/thrift/com/twitter/zipkin/zipkinCore.thrift). Why not add that as a column to the UI?

[codefromthecrypt]

I'm over quota rehashing this, but trying again. binaryannotation.host is sometimes set, so yes when it is present the UI could display it. That display choice will cost code, for example whether to log the same pair twice when values agree or not. This type of code often breaks. Others have mentioned on this thread that they filter out duplicates, just like we do now. Also, knowing the host is a special case. Even if a feature is possible to make, that doesn't mean we should. Finally, if we do this and choose to make the model use string-string tags (which dont include host) as discussed in the past, we will break the feature. Regardless, it is so unpopular that it doesn't deserve this kind of attention. Either someone should implement it in a supportable way, or close it. No free lunch

[codefromthecrypt]

Shorter why not:

Widening the UI from key-value to (key, value, host) causes the following.

• Clutter for those who perceive tags as unique per key. Ex multiple rows
  saying http.method=GET for each host
• More code to maintain if we choose to be less cluttery (ex suppress on
  identical key/value, etc)
• More difficult decisions later, especially around data structures to
  represent tags. This is what i am moat concerned with.

Right now, the path is clear as "binary annotation" and all its complexity
isn't presented to the user in a way that would prevent us from moving to
plain string-string.

As both someone who often has to fix bugs in code like this, yet also one
interested in actually improving our model to prevent them.. I really don't
like this feature as it will almost certainly cause burden for very little
gain.

[abesto]

Got it. Thanks for the explanation; with those in mind, it does sound like the effort outweighs the gain for now.

[codefromthecrypt]

Thanks for thinking it through.. Bus factor increase!

[codefromthecrypt]

FYI current workaround when people are sending the same key from multiple services is to look at the json. Looking at the json will be easier if anyone implements the json button #1060

[codefromthecrypt]

Here's some thoughts about choices we can make besides closing this issue.

Display service name in binary annotations knowing what we know

We have discussed this issue, and know that many instrumentation projects dodge duplicating binary annotation key/value pairs in the same span. We also know the model won't likely support multimaps with endpoints in the future. We could choose to make this change, inverting the UI element to showing more detail as opposed to less, eventhough we know the constraints of this.

If we did something like this, we should explicitly agree. For example, multiple people. When it gets to implementation, we should make the UI element consistent, either by emulating the style of annotation rows, or changing the style of annotation rows. For this reason, we should be careful to review pull requests carefully. Ex. Incomplete screen shots can hide inconsistency.

Make it easier to fork the UI

The UI no longer has any scala dependency etc. This particular change is not the only one that sites may want to customize. For example, they may want to change the titles, favicons etc. Rather than making everyone see an edge case, we can encourage forks, possibly by moving the UI into a separate repo. When we work in such a model, it means we can delay loaded topics like this until they become more widely requested.

your idea here

[abesto]

I love the idea of making the UI customizable.

Another way would be bringing in features like this in a configurable way, linking to the discussion of the trade-offs involved. This approach could nicely support bringing in features from forks: keep the default experience consistent and always correct, but let users choose their trade-offs. Of course that adds overhead in maintenance, or rather instability; we'll never ever test all 600 combinations of feature flags. Initially I'd solve that by adding cautionary comments and documentation. Over time we could start supplying well-tested configuration presets.

All in all, I'm not sure it's a good trade-off, but it's a nice idea to play with.

[basvanbeek]

Considered fixed as per #1867.

dupes are removed when both key and value match. duped key with different values will continue to be reported as it might signal important info and we don't know which one is correct if not both.