Stop using custom annotations and move them into tags

[jcchavezs]

Is your feature request related to a problem? Please describe.
Zipkin provides a set of idiomatic annotations for server/client events described in https://zipkin.io/pages/instrumenting.html#core-data-structures. The current annotations Received xxx bytes are more suitable with tags as the timestamp does not provide any value more than the server start and server finish.

Describe the solution you'd like
Move the request/response size data into idiomatic tags and only keep core annotations:

/** The size of the non-empty HTTP request body, in bytes. Ex. "16384" * * Large uploads can exceed limits or contribute directly to latency. */
const string HTTP_REQUEST_SIZE = "http.request.size" 

/** The size of the non-empty HTTP response body, in bytes. Ex. "16384" * * Large downloads can exceed limits or contribute directly to latency. */
const string HTTP_RESPONSE_SIZE = "http.response.size"

I am more than happy to come up with a PR for this.

Ping @adriancole @basvanbeek @bogdandrutu

[codefromthecrypt]

yep as it is there are timestamped annotations with embedded variables which unnecessarily adds indexing load (aka dollars)

[bogdandrutu]

@jcchavezs in case of a stream protocol we can have more than one annotation. The timestamp is important for few reasons:

1. Determines the delay between the moment when the RPC was started and when the message was sent (in case of some queuing in the RPC client).
2. For streams it is important to know when different packages are sent.

[codefromthecrypt]

these are unwary single message requests as far as I can tell. the problem besides redundancy is embedding the byte sizes which makes these annotations high cardinality with no added value vs tags

…

On Thu, May 9, 2019, 2:15 AM Bogdan Drutu ***@***.***> wrote: @jcchavezs <https://github.com/jcchavezs> in case of a stream protocol we can have more than one annotation. The timestamp is important for few reasons: 1. Determines the delay between the moment when the RPC was started and when the message was sent (in case of some queuing in the RPC client). 2. For streams it is important to know when different packages are sent. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV2FOVGAC3Z4BVHNKOTPUMKD5ANCNFSM4HLPOJFA> .

[jcchavezs]

So @bogdandrutu are the size really necessary? if they are not, @adriancole are there idiomatic annotations for such a thing? is there any discussion around that? Something like ssp for Server Send Package or something like that?

[bogdandrutu]

Size may be dropped if that causes problems for Zipkin

[codefromthecrypt]

in the case of unary where there is only one size noted (for request and response) and timing matches start/duration, there's really only a question of whether or not to add these things as tags.

when the timing isn't matching exactly, there are "ws" "wr" annotations, and also some fragment annotations as well. In any case the annotations do not concatenate size in their names.

So basically I would suggest that you can consider..

• whether to include request/response size or not as tags
• whether or not to add delayed scheduling of unary onto wire as "ws" "wr" or not
• whether or not to add fragment annotations https://github.com/apache/incubator-zipkin-api/blob/master/thrift/zipkinCore.thrift#L120-L139

in any case, as long as size is not embedded in annotation names, it is not going to be bad.