187 B3 Headers Support #188
Conversation
| LOWER_HEX_16 = re.compile(r"\A[0-9a-f]{16}\Z") | ||
| LOWER_HEX_32 = re.compile(r"\A[0-9a-f]{32}\Z") | ||
|
|
||
| LOWER_HEX_16_BYTES = re.compile(b"\A[0-9a-f]{16}\Z") |
| @@ -0,0 +1,122 @@ | |||
| import re | |||
There was a problem hiding this comment.
nit: this file is missing the from __future__ stuff which should be standard in all baseplate files.
| @@ -80,7 +82,8 @@ def getHandlerContext(self, fn_name, server_context): | |||
| # just attach the raw context so it gets passed on | |||
| # downstream even if we don't know how to handle it. | |||
| context.raw_request_context = edge_payload | |||
| except (KeyError, ValueError): | |||
| except (KeyError, ValueError) as exc: | |||
| logger.debug(exc) | |||
| LOWER_HEX_32_BYTES = re.compile(b"\A[0-9a-f]{32}\Z") | ||
|
|
||
|
|
||
| class TraceHTTPHeaders(object): |
There was a problem hiding this comment.
is there any reason for this to be a class? rather than doing inheritance shenanigans it could just be a function that returns a namedtuple (which also saves you from using ["dict lookup"] style stuff) and we can have two variants for thrift and http.
There was a problem hiding this comment.
in fact, it feels a little like we're building an object (TraceHTTPHeaders) just to turn it into another object (TraceInfo). can we just build the TraceInfo directly?
| if header_name in headers: | ||
| values.append(headers[header_name]) | ||
|
|
||
| if len(values) == 0: |
There was a problem hiding this comment.
nit: if not values: is more pythonic
| continue | ||
| # make sure all found values are equal | ||
| # http://stackoverflow.com/q/3844948/ | ||
| elif values.count(values[0]) != len(values): |
There was a problem hiding this comment.
might be a little easier to understand as elif not all(value == values[0] for value in values):
| # make sure all found values are equal | ||
| # http://stackoverflow.com/q/3844948/ | ||
| elif values.count(values[0]) != len(values): | ||
| raise ValueError("Conflicting values found for %s header(s)", header_names) |
There was a problem hiding this comment.
i don't think valueerror does any string formatting does it?
baseplate/core.py
Outdated
| @@ -108,31 +108,13 @@ def new(cls): | |||
| def from_upstream(cls, trace_id, parent_id, span_id, sampled, flags): | |||
| """Build a TraceInfo from individual headers. | |||
|
|
|||
| :param int trace_id: The ID of the trace. | |||
There was a problem hiding this comment.
what's rollout for this look like if something suddenly starts sending long stringy IDs to downstream services that haven't been upgraded yet? are all the baseplate observers we have OK with this changing from int to str?
|
|
||
| HEX_16_RE = LOWER_HEX_16 | ||
| HEX_32_RE = LOWER_HEX_32 | ||
| VALID_SAMPLED_VALUES = ("0", "1") |
There was a problem hiding this comment.
the fact that they're strings on the wire feels like something the app shouldn't have to care about. can we decode the trace IDs into consistent integers within the application always? and the "sampled" flag into a boolean? it feels painful to make the app have to check against == "1" rather than just testing truthiness.
|
|
||
| """ | ||
| HEADER_NAMES = { | ||
| "trace_id": ["X-Trace", "X-B3-TraceId"], |
There was a problem hiding this comment.
Should we use tuples rather than lists for these? Tuples are a little smaller and immutable.
There was a problem hiding this comment.
Also, the way these are ordered will favor the old header format over the new, maybe we should put the new one first?
|
|
||
|
|
||
| class TraceHTTPHeaders(object): | ||
| """Helper class for trace headers in an HTTP call. |
There was a problem hiding this comment.
If this is it for the docstring, I think you can fit the closing """ on the same line.
|
💇 @spladug @pacejackson Thanks for the feedback. My first approach was way too heavy handed and over engineered. I tried to greatly simplify things and focus solely on getting Baseplate to look at B3 headers in addition to those it already supported. |
baseplate/integration/pyramid.py
Outdated
| flags=flags, | ||
| ) | ||
|
|
||
| trace_info = TraceInfo.from_http_headers(request.headers) |
There was a problem hiding this comment.
It's kinda a bummer to make TraceInfo have to be aware of the different protocols we're using; if we added more down the line that'd mean more direct modifications in that class rather than just adding the new protocol definitions. The core stuff is meant to be the idealized form of the tracing and then these integrations are where that meets the actual wire. The centralized extraction logic could potentially be exposed as a helper that these integrations use though?
|
💇 |
| @@ -135,6 +136,62 @@ def from_upstream(cls, trace_id, parent_id, span_id, sampled, flags): | |||
|
|
|||
| return cls(trace_id, parent_id, span_id, sampled, flags) | |||
|
|
|||
| @classmethod | |||
| def extract_upstream_header_values(cls, upstream_header_names, headers): | |||
There was a problem hiding this comment.
Would it make sense just to merge this into from_upstream ? I don't think we'd ever use one without the other?
There was a problem hiding this comment.
Yeah... I wanted to do that but there are subtle differences in how the two protocols handle the header values due to Thrift operating in bytes and HTTP using strings.
https://github.com/reddit/baseplate/pull/188/files#diff-db4cf04c08224f72cb9627e1f9866bbcR121
https://github.com/reddit/baseplate/pull/188/files#diff-d1382533c32ca95376190e31679acc05R162
Some type checking could be done. Or possibly a generic
True if extraced_values["sampled"] in ("1", b"1")
but this started to feel protocol specific again. If others are added, other protocol specific edge cases could arise making the from_upstream method kind of gnarly. I'm happy to take a stab at it though, if you'd like.
There was a problem hiding this comment.
👍 alrighty, makes sense. thanks
| if not values: | ||
| continue | ||
| elif not all(value == values[0] for value in values): | ||
| raise ValueError("Conflicting values found for %s header(s)".format(header_names)) |
There was a problem hiding this comment.
do we want to crash the whole request if the headers are iffy? (i could see either answer being reasonable)
There was a problem hiding this comment.
I raised this to follow what is being done in new. Where this is called is wrapped so the request doesn't get nuked but rather the trace data is not propagated.
Add support for B3 trace headers whil continuing to support the old trace headers. This change aims to allow both both B3 and the old headers to be used as long as they contain identical values. For example: ``` X-Trace: 1234567890 X-B3-TraceId: 1234567890 ``` Fixes reddit#187
Add support for B3 trace headers whil continuing to support the old trace headers. This change aims to allow both both B3 and the old headers to be used as long as they contain identical values. For example: ``` X-Trace: 1234567890 X-B3-TraceId: 1234567890 ``` Fixes #187
Add support for B3 trace headers whil continuing to support the old
trace headers. This change aims to allow both both B3 and the old
headers to be used as long as they contain identical values. For
example:
Fixes #187