Make tracestate a list instead of a map to preserve ordering.#84
Conversation
| // | ||
| // See the https://github.com/w3c/distributed-tracing for more details about this field. | ||
| map<string, string> tracestate = 15; | ||
| repeated TraceStateEntry trace_state = 15; |
There was a problem hiding this comment.
nit: trace_state -> trace_states?
c2fd25b to
cb0a16c
Compare
|
Blocked by one decision in census-instrumentation/opencensus-specs#168. |
cb0a16c to
24480b6
Compare
| // | ||
| // See the https://github.com/w3c/distributed-tracing for more details about this field. | ||
| map<string, string> tracestate = 15; | ||
| message Tracestate { |
There was a problem hiding this comment.
Minor nit, why don't we break this out into a message of its own, that is, a top-level definition instead of an inlined definition here?
|
yes tracestate |
| } | ||
|
|
||
| // The Tracestate on the span. | ||
| Tracestate tracestate = 15; |
There was a problem hiding this comment.
another option is to have a string and not parse it except to remove your incoming tracestate. OTOH what you have is more flexible for future changes. As mentioned in the spec review "For multi-tenant vendors scenarios '@' sign can be used to prefix vendor name." is highly speculative, and the format isn't well thought through. Usually '@' is not a character promoted for use for various reasons I can think of a few. I wouldn't advertise this in good faith.
There was a problem hiding this comment.
ps option for ridding @ is just deleting the advice (ideally also from where it was copy/pasted from). We should not encourage characters that routinely require escaping.
|
CC: @lmolkova |
One thing I want to confirm is the name tracestate vs trace_state. I don't like the fact that tracestate is not a real word so all the time the spelling checker triggers on this.