Implementing W3C TraceContext (fixes #116)

[toumorokoshi]

This introduces a w3c TraceContext propagator, primarily inspired by opencensus.

[toumorokoshi]

Currently have one typing issue I'm sorting through, but the rest should be working as intended.

[reyang]

traceparent_string = "00-{:032x}-{:016x}-{:02x}".format(trace_id, span_id, trace_options)

[reyang]

I suggest that we use "".format(...) instead of %.

[reyang]

I guess we want to pop the oldest value and take the new one?

[untitaker]

Yeah, by this logic the tracestate becomes immutable once it reaches the limit. However, overwriting an existing key does not change length and should work IMO.

Also I am reading the limit in the spec as a MAY, so IMO it would make sense to apply the limit during extraction only (but not prevent people from sending large tracestate).

[Oberon00]

IMHO we should just keep TraceState being a dictionary (or ordered dictionary) and not entangle it with the W3C spec more closely than required. So the formatter would just strip superfluous values on extract (and maybe inject).

[toumorokoshi]

sounds good. @unitaker your point is valid: existing keys should be overrideable. Can add.

@reyang the spec is ambiguous as to what should be done when a maximum is reached. I think either could be an issue.

@Oberon00 that is a good point. will move it to the extract / injection part. Or probably remove that limit entirely.

All that said it's a good point that this is an optional choice. I'd probably just vote to remove this restriction in the basic implementation: I worry about people not knowing the spec well enough and running into a tracecontext member limit.

[reyang]

@toumorokoshi the spec is clear about this:

Then entries SHOULD be removed starting from the end of tracestate.
Note that other truncation strategies like safe list entries, blocked list entries,
or size-based truncation MAY be used, but are highly discouraged.

[reyang]

Overall looks good, I've left some minor suggestions, none of them are blocking though.
Let's try to get this reviewed and merged, and then work on smaller PR to polish it.

[Oberon00]

Looking good to me 👍 , just nitpicks.

[Oberon00]

I'd prefer not using regular expressions for this, as the format should be simple enough to process with "".split("-"). Also, I think implementing https://www.w3.org/TR/trace-context/#versioning-of-traceparent would be easier that way. But that is no strong opinion.

[toumorokoshi]

that would require checking the validity of the individual values, but I can take a look. regex is very efficient for these type of operations.

I was just curious: the spec looks like it provides no guarantees around the format of future traces. so the most we could do is split on first "-", check the version, and pass-through or ignore if the version is above 00. would that be the right forward-compatible behavior?

[reyang]

I think regex is the most efficient way (scientifically) to handle string parsing - if implemented using the correct Ken Thompson DFA/NFA. No strong opinion though :)

[Oberon00]

Incomplete sentence.

[Oberon00]

Suggested change

	match = re.search(cls._TRACEPARENT_HEADER_FORMAT_RE, header[0])
	match = cls._TRACEPARENT_HEADER_FORMAT_RE.fullmatch(header[0])

• If you have a regex object, I think it looks better to use the method instead of the the re module's function (I did not know that this even works)
• search is bad because it may skip over any junk before the content we are searching for. match is better because it ignores only junk after the end but fullmatch is probably what we should use here.

EDIT: I see now you have used ^$ in the regex, but IMHO fullmatch is still more clear than search (and maybe more efficient).

[Oberon00]

I don't think this check is very useful here. I'd only check after constructing span_context at the end, that way we can do integer comparisons.

[toumorokoshi]

That seems ok. The one point there is we have to reject all values if either is invalid, as well as this skips some level of processing. But should be fine.

[Oberon00]

Suggested change

	return ",".join(map(lambda key: key + "=" + tracestate[key], tracestate))
	return ",".join(key + "=" + tracestate[key] for key in tracestate)

or

Suggested change

	return ",".join(map(lambda key: key + "=" + tracestate[key], tracestate))
	return ",".join(key + "=" + value for key, value in tracestate.items())

[Oberon00]

I'm not happy about that but it seems the best we can do since there is no generic OrderedDict. (And all dicts being ordered is only guaranteed since 3.7 and implemented since 3.6).

[toumorokoshi]

there's no need for this anymore since we're making the "TraceState" object in otel more generic.

[Oberon00]

Since we now allow any key/value-types in the tracestate, should we use str(key)/str(value) maybe?

[toumorokoshi]

this is a good point. I'll probably add this logic in a subsequent pr.

[Oberon00]

Do these really belong here? I think TraceState is not tightly bound to the W3C trace state.

[toumorokoshi]

removed this logic entirely.

[toumorokoshi]

I'm merging this now to unblock. Open to any further suggestions as well.

[c24t]

Can you wrap these comments?