How should we render annotations that are json?

[codefromthecrypt]

Right now, we render annotations that hold json as string literals.

Ex. if I do this in finagle

Trace.record("{\"event\": \"soft error\", \"type\": \"cache timeout\", \"waited.millis\": 1500}");

Or if I use opentracing's logKV api, this renders like so..

Presumably, we could do better, but I'm not sure how. Any ideas?

cc @basvanbeek @yurishkuro @bensigelman

[rogeralsing]

Is there any way to detect that the annotation is Json? the normal annotations don't have any "type", right?
So we would have to do a client side check if the string can be parsed, right?

[codefromthecrypt]

I was looking at the type like this..

https://github.com/openzipkin/zipkin/pull/1307/files

[eirslett]

https://codegeekz.com/15-best-jquery-json-plugins/

[eirslett]

Something like an interactive JSON viewer where nodes can be expanded and collapsed?

[basvanbeek]

I would like that viewer to be inline (expandable/collapsible) inside the annotation detail pane.

This would also work well for OpenTracing Key/Value pairs

[rogeralsing]

I have some early progress on this, at least the JSON is detected and rendered in its own way.

So how far should we push this?
Should we go for a full JSON formatter like this https://jsonformatter.curiousconcept.com/
Or, just apply pretty print and use e.g. Highlight JS on the results?

[rogeralsing]

Meh, couldnt help it, I'm building my own JSON viewer in Angular.

Going to throw some expander buttons at the arrays and objects.

[codefromthecrypt]

At first glance, since the view is tabular, seems nice to have something
like.

// for some limit of column length, render as key/value (assume it isn't
nested as there's currently no use case for that)
key1=value1 key2=value2...

Then, you can click on the above to expand a json tree view.

Feels like it would be less distracting that way, particularly as rendering
json is currently a niche functionality (as some users who accept key/value
in tracers will expand it to a string as opposed to sending as raw json
anyway)

[codefromthecrypt]

ps unless you're really interested I wouldn't sink too much time into this. It is a speculative feature, which hopefully is more a workaround. Ex quoted json doesn't work for search or anything else in zipkin.

If we ever supported anything like this properly, it would be far more coherent and far more searchable as a timestamp column on a binary annotation. Most of our backends have a timestamp on each searchable binary annotation row anyway.

I highly doubt our next models will include Map -> timestamp data structures as the span model is already complex. openzipkin/zipkin#939

[codefromthecrypt]

If we choose to support quoted json client-side, let's keep scope down. opentracing has tags (which map to binary annotations) and the new thing is that annotations might be a single-level map rendered as quoted json. Single-level aren't allowed to be nested, so you wouldn't have "a":[1,2,3]

This in mind, they could just as well be rendered the same as our binary annotations, except possibly adding the timestamp of the annotation that encloses them.

I still shudder at the abuse of the string field

[codefromthecrypt]

ps added more rationale here, hoping people resist the temptation to pollute zipkin with stringified json cc opentracing advocate @basvanbeek

openzipkin/openzipkin.github.io#52 (comment)

[eirslett]

When you do searches in Elasticsearch, the query is JSON, not SQL. And that could be handy to have as a binary annotation, right?

[codefromthecrypt]

@eirslett elasticsearch queries specialize in natural json fields. they cannot navigate into quoted json naturally.

For example, the following two json:

    {
      "timestamp": 1474934400200000,
      "value": "{\n  \"type\": \"error\",\n  \"kind\": \"client\",\n  \"class\": \"SocketException\",\n  \"message\": \"socket closed\"\n}"
    }

    {
      "timestamp": 1474934400200000,
      "value": "Client Receive Error: SocketException: socket closed"
    }

The latter is easier to query with elasticsearch (sql, or cql) operators, as it is a plain string, vs quoted json.

Do you disagree?

https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html

[codefromthecrypt]

Ex. I know of no syntax to navigate into quoted json, like value.kind:"client" Maybe there's a "fuzzy json" mechanism I'm not aware of..

[codefromthecrypt]

@eirslett I think I answered the wrong question :)

You aren't asking about how to query annotations in json using elasticsearch, you are asking about when you are tracing elasticsearch and the query was represented in json and a binary annotation. Sorry, I got confused.

Yes, of course if a binary annotation value itself is json, or any other kind of string, there's a use case for displaying it. I think we ran into that before. Binary annotations naturally have a key, so that's actually something we can handle even better as time goes on.

This issue is about annotations, not binary annotations. Ex. embedding quoted json as the annotation value. I think this practice is more questionable.

[rogeralsing]

So where are we on this topic? should we have special rendering of json?

[codefromthecrypt]

IMHO definitely we need to render binary annotation values properly as json (ex arbitrary levels of nesting). This was a feature that existed before. On how to render keys (ex annotation.value or binaryannotation.key), I'd prefer to special-case, since the only user is opentracing. In opentracing, they have restricted to single-level depth, which means we should be able to splat these values across the column. So, here's the suggestion if we are to handle it at all. if there's only one value, emit that. ex unwrap {"event": "foo"} as "foo" if there's multiple values, emit them as key/value "key1=value1 key2=value2...", ideally ordered. Rationale is reading "Server Receive" some nested thing "Server Send" is very visually distracting Also, I really hope the usage of quoted json gets contained, and it feels tech debt to sink too much effort something like that.