Cleanup TraceFlags Alternatives

[bogdandrutu]

There are some options proposed:

1. Do nothing - the API will be very different compared with TraceId/SpanId and inconsistent. Methods to copy on SpanContext, methods to parse on TraceFlags, etc.
2. Change how by default SpanContext exposes TraceFlags and use hex representation ([FEEDBACK REQUIRED] Change TraceFlags to follow the ids and be hex by default #2698) . There are nice things about this but also complications: a) one more source of invalid part in the SpanContext if the flags are not lowercase hex; b) checking for sampling will be a bit more expensive. This version was developed more into having 2' (which is more or less internal detail evolution [FEEDBACK REQUIRED] Traceflagsref precompute #2706)
3. Do not change SpanContext exposed representation for TraceFlags ([FEEDBACK REQUIRED] Cleanup TraceFlags, make it consistent with ids, but use byte as default #2701) and expose hex representation as a String/CharSequence.
4. Do not change SpanContext exposed representation for TraceFlags ([FEEDBACK REQUIRED] Cleanup TraceFlags, make it consistent with ids, but use byte as default #2703) and expose hex representation as two characters kind of similar with the logs for TraceId.
5. Expose it as a class (Change TraceFlags to be a class, expose all helpers on the class itself. #2709).

We should decide on one of the options ASAP and implement (if needed).

[bogdandrutu]

/cc @anuraaga, @jkwatson, @trask, @iNikem, @Oberon00 I would like to hear your opinions here.

[jkwatson]

As commented on the PR, I think I prefer option (2) (#2698), but not by a large margin. I think my feeling is mostly about the size of the change, rather than the precise details of the change itself. Clearly, either will work.

[bogdandrutu]

The smallest size will probably be 3 or 4 because they don't touch SpanContext that much (only remove a function). Keep in mind that in the second PR I have both options maybe I should split to show the size

[jkwatson]

The smallest size will probably be 3 or 4 because they don't touch SpanContext that much (only remove a function). Keep in mind that in the second PR I have both options maybe I should split to show the size

I think it was the requirement of having firstHexCharacter secondHexCharacter which felt "bigger" and awkward to me.

[bogdandrutu]

That is option 4. I will split it to avoid confusion.

[jkwatson]

oh shoot. I missed that the 2 were independent. :)

[bogdandrutu]

@jkwatson done, you can now see the 2 options separately :)

[anuraaga]

I haven't gotten to do a detailed look through the PRs yet, but #2 seems reasonable to me - users would call isSampled() anyways, and it seems the most consistent for propagators, which are probably the main users of getTraceFlags()

[jkwatson]

After looking back and forth at the 4 options like 6 times, I think I slightly prefer #2701 (option 3), but really, I think the number of users for these changes are so miniscule that it doesn't matter all that much to me.

[bogdandrutu]

@anuraaga I expect users to call isRecording on the Span not the isSampled, so for users that only instrument code is probably a noop

[bogdandrutu]

@jkwatson options 3 and 4 are the most performant, options so I slightly prefer one of these, but option 2 is slightly more consistent with the IDs. If I were to vote I would go for 3,4,2 (I really don't want to ignore the cleanup so no vote for 1)

[anuraaga]

Modification of 2, since there are only 255 possibilities, we could have 255 * (class TraceFlag { String hex; byte val; boolean sampled; }) and just have SpanContext forward to the appropriate. So expose trace flags as hex by default, return isSampled directly from that field, etc. I'd be surprised if we have to sacrifice performance for any option given the cap of 255.

[bogdandrutu]

The problem is that it is very fast to search in that 255 list by byte number but to search by string is required to either transform the string into number at that point you just do a bit and or have a map which is slower.

A 2' option where we return the class (one of the 255) is as fast as the the bytes. But if users will have a String in their hand is hard to lookup by that in the list of classes.

[bogdandrutu]

So we can do a 2' where we return a class TraceFlags and pre-construct all of them. That is another possibility

[bogdandrutu]

Decision was to move forward with option 5