add doc on zipkin exporter for converting oc spans

[tsloughter]

This currently only describes the annotations but I can expand to cover the rest of a span, either in this same PR or this can be merged and I'll send another to fill out the rest.

I'm also curious if zipkin wouldn't be better served if message events were included twice. Once as is described in this document to include the id and sizes and an additional time with just the type, but in zipkin's form.

From the example in the document:

{"time": 67890,
  "message_event": {"type": "SENT", 
                    "id": 5,
                    "compressed_size": 25,
                    "uncompressed_size": 38}}

Would result in two annotations, one with value of just "ss" or "cs" for "server sent" or "client sent" (problem is I don't know how it would know if it is the server or client...):

{"timestamp": 67890,
 "value": "ss"}

{"timestamp": 67890,
 "value": "MessageEvent:{type=Sent, id=5, compressed_size=25, uncompressed_size=38}"}

[tsloughter]

I can't add reviewers so will just tag @adriancole directly :)

[codefromthecrypt]

One thing very important to users is value for their money. Annotations cause index load especially routine ones such as sent events. concatenated values are not searchable in any way, and the index number in the events is near meaningless except I suppose in the case timestamp is ambiguous. The difference between compressed and uncompressed size, even if added as a tag, is not likely helpful as is. For example, we have http.response_size etc which is what folks would be looking for if they were looking for it. This would be the wire size (presumably compressed size in this case).

TL;DR; I would wait quite a bit before adding anything redundant by default. While it seems neat to add a lot of things, we already have quite a few folks interested in reducing the data in their spans :) This is where span policy probably should come into play and where conservative defaults should win until that's available

cc also @jcchavez @basvanbeek

[basvanbeek]

This is very similar to the problems with OpenTracing's LogEvents. I would appreciate standard middlewares / defaults to not have too many of these elaborate span details as outlined by @adriancole.

If we do end up with these than the next best step would be to allow the consumer to chose the serialization logic for attributes in Annotations and MessageEvents.

So I'm struggling a bit with the "Sent" MessageEvent example. Especially with respect to what sent means.

In Zipkin v2 API we don't use the CS, SR, SS, CR annotations anymore. At least OC Go exporter uses v2 as it uses the data model from the zipkin-go tracer which is v2 only, not sure of other language's exporters. These annotations are replaced by span start time, span duration and span kind (client / server).

CS == span start time for kind=client SR == span start time for kind=server SS == span start time + duration for kind = server CR == span start time + duration for kind= client

We do still have the concept of WS (wire sent) and WR (wire receive) annotations. In a lot of situations (especially bog standard RPC ones) using these annotations has little value as the instrumented frameworks or transports often don't make it possible to clearly distinguish between span start/end and on-the-wire events. Or the time between them is negligible in the broader timeline.

So if the events map more with CS, SR, SS, CR we might want to ignore them if no attributes are found or just convert the attributes to Zipkin tags (untimed key-value pairs) for specific MessageEvents and not serialize as Zipkin Annotations.

On the topic of serialization we should probably allow consumers to select the method of serialization of Annotation and MessageEvent attributes (ex. logfmt, json, drop). We have provided the option to select serialization method in our zipkin-go-opentracing implementation and people often opt for dropping the attributes as value is little and noise is large.

[tsloughter]

Link to the zipkin v2 spec?

[basvanbeek]

Check out: https://zipkin.io/zipkin-api/#/

And more details / conversations / issues at the API project: https://github.com/openzipkin/zipkin-api

[tsloughter]

Ah ok, is the model I was looking at.

I can update the spec to include options for choosing json or drop for attributes.

[codefromthecrypt]

I feel we are specifying things way too early and with very few involved. Why wouldn't we just have something library-specific until there are multiple people wanting to do something?

[tsloughter]

If we do one library specific version now, without any documentation, then there will be a second (different) library specific version at some point in the future.

Even if we don't merge this now it is useful as an open PR for that future person who can then comment here that this is something they need :).

Also, the person who was asking for this in the Erlang lib wasn't expecting the Attributes part, so should consider removing that?

Does the opencensus spec specifiy that the attributes in an annotation are tagged with the string "Attributes". At least in zipkin I expected to see the description, then the attributes as simple key=value pairs.

But I'm fine waiting to have anything merged.

[codefromthecrypt]

it is an odd thing the culture we've made where there is pressure to add things to a spec repo before there's any practice. You aren't the first to do this, it happens routinely.. just you're the first one I noticed after restarting watching the repo.

IMHO it is really bad behaviour to open long term PRs. It isn't fair pressure or correct in any way to presume something with no practice should be specified as a practice. Especially when two folks have advised against excess recording by default. We have to remember this is a low-level library so can create a bunch of what might be perceived as junk.

Is there a way you can just close this, experiment on your own and consider re-opening as a spec issue if..

A. it works out under load not in demo projects B. becomes popular with more than a couple people C. cavaots or opt-out stuff are coherently addressed so that low-value load isn't forced on the ecosystem

While things are new, folks like to experiment and that's good. We just need to still keep in mind what long term effects will be and if it is a good policy to keep doing this spec and merge before practice thing.