-
Notifications
You must be signed in to change notification settings - Fork 55
feat: trace improvements #450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9fe2d8a
3f464b5
c77657a
310ae13
98bd28c
93d0e61
e8785cb
3fd0899
899dd93
cb8bdfb
d652588
0563239
0d53025
c9bc772
d9b4f0a
8662338
a874e9a
0286522
b30f9e7
7e521db
e395a38
438b3ee
a6c6047
bcbb318
b3acb10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,11 +27,13 @@ | |
from google.cloud.logging_v2.handlers.middleware.request import _get_django_request | ||
|
||
_DJANGO_CONTENT_LENGTH = "CONTENT_LENGTH" | ||
_DJANGO_TRACE_HEADER = "HTTP_X_CLOUD_TRACE_CONTEXT" | ||
_DJANGO_XCLOUD_TRACE_HEADER = "HTTP_X_CLOUD_TRACE_CONTEXT" | ||
_DJANGO_TRACEPARENT = "HTTP_TRACEPARENT" | ||
_DJANGO_USERAGENT_HEADER = "HTTP_USER_AGENT" | ||
_DJANGO_REMOTE_ADDR_HEADER = "REMOTE_ADDR" | ||
_DJANGO_REFERER_HEADER = "HTTP_REFERER" | ||
_FLASK_TRACE_HEADER = "X_CLOUD_TRACE_CONTEXT" | ||
_FLASK_XCLOUD_TRACE_HEADER = "X_CLOUD_TRACE_CONTEXT" | ||
_FLASK_TRACEPARENT = "TRACEPARENT" | ||
_PROTOCOL_HEADER = "SERVER_PROTOCOL" | ||
|
||
|
||
|
@@ -62,13 +64,12 @@ def get_request_data_from_flask(): | |
"""Get http_request and trace data from flask request headers. | ||
|
||
Returns: | ||
Tuple[Optional[dict], Optional[str], Optional[str]]: | ||
Data related to the current http request, trace_id, and span_id for | ||
the request. All fields will be None if a django request isn't | ||
found. | ||
Tuple[Optional[dict], Optional[str], Optional[str], bool]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: shouldn't bool be optional too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the LogEntry spec says if the sampling decision is uncertain, you should default the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, the code immediately below returns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good catch! I started with it being optional before switching. I'll look through the code again |
||
Data related to the current http request, trace_id, span_id and trace_sampled | ||
for the request. All fields will be None if a django request isn't found. | ||
""" | ||
if flask is None or not flask.request: | ||
return None, None, None | ||
return None, None, None, False | ||
|
||
# build http_request | ||
http_request = { | ||
|
@@ -79,25 +80,29 @@ def get_request_data_from_flask(): | |
} | ||
|
||
# find trace id and span id | ||
header = flask.request.headers.get(_FLASK_TRACE_HEADER) | ||
trace_id, span_id = _parse_trace_span(header) | ||
# first check for w3c traceparent header | ||
header = flask.request.headers.get(_FLASK_TRACEPARENT) | ||
trace_id, span_id, trace_sampled = _parse_trace_parent(header) | ||
if trace_id is None: | ||
# traceparent not found. look for xcloud_trace_context header | ||
header = flask.request.headers.get(_FLASK_XCLOUD_TRACE_HEADER) | ||
trace_id, span_id, trace_sampled = _parse_xcloud_trace(header) | ||
|
||
return http_request, trace_id, span_id | ||
return http_request, trace_id, span_id, trace_sampled | ||
|
||
|
||
def get_request_data_from_django(): | ||
"""Get http_request and trace data from django request headers. | ||
|
||
Returns: | ||
Tuple[Optional[dict], Optional[str], Optional[str]]: | ||
Data related to the current http request, trace_id, and span_id for | ||
the request. All fields will be None if a django request isn't | ||
found. | ||
Tuple[Optional[dict], Optional[str], Optional[str], bool]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: same question about Optional[] here |
||
Data related to the current http request, trace_id, span_id, and trace_sampled | ||
for the request. All fields will be None if a django request isn't found. | ||
""" | ||
request = _get_django_request() | ||
|
||
if request is None: | ||
return None, None, None | ||
return None, None, None, False | ||
|
||
# build http_request | ||
http_request = { | ||
|
@@ -108,54 +113,94 @@ def get_request_data_from_django(): | |
} | ||
|
||
# find trace id and span id | ||
header = request.META.get(_DJANGO_TRACE_HEADER) | ||
trace_id, span_id = _parse_trace_span(header) | ||
# first check for w3c traceparent header | ||
header = request.META.get(_DJANGO_TRACEPARENT) | ||
trace_id, span_id, trace_sampled = _parse_trace_parent(header) | ||
if trace_id is None: | ||
# traceparent not found. look for xcloud_trace_context header | ||
header = request.META.get(_DJANGO_XCLOUD_TRACE_HEADER) | ||
trace_id, span_id, trace_sampled = _parse_xcloud_trace(header) | ||
|
||
return http_request, trace_id, span_id | ||
return http_request, trace_id, span_id, trace_sampled | ||
|
||
|
||
def _parse_trace_span(header): | ||
def _parse_trace_parent(header): | ||
"""Given a w3 traceparent header, extract the trace and span ids. | ||
For more information see https://www.w3.org/TR/trace-context/ | ||
|
||
Args: | ||
header (str): the string extracted from the traceparent header | ||
example: 00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01 | ||
Returns: | ||
Tuple[Optional[dict], Optional[str], bool]: | ||
The trace_id, span_id and trace_sampled extracted from the header | ||
Each field will be None if header can't be parsed in expected format. | ||
""" | ||
trace_id = span_id = None | ||
trace_sampled = False | ||
# see https://www.w3.org/TR/trace-context/ for W3C traceparent format | ||
if header: | ||
try: | ||
VERSION_PART = r"(?!ff)[a-f\d]{2}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: right now the version must be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are you handling other versions? Just ignoring anything other than It looks like nodejs is assuming forward-compatibility, based on this part of the spec ("The current version of this specification assumes that future versions of the traceparent header will be additive to the current one."). I'm fine with locking to version 00 for now though if we decide that's safer. Let me know if you have thoughts. |
||
TRACE_ID_PART = r"(?![0]{32})[a-f\d]{32}" | ||
PARENT_ID_PART = r"(?![0]{16})[a-f\d]{16}" | ||
FLAGS_PART = r"[a-f\d]{2}" | ||
regex = f"^\\s?({VERSION_PART})-({TRACE_ID_PART})-({PARENT_ID_PART})-({FLAGS_PART})(-.*)?\\s?$" | ||
match = re.match(regex, header) | ||
trace_id = match.group(2) | ||
span_id = match.group(3) | ||
# trace-flag component is an 8-bit bit field. Read as an int | ||
int_flag = int(match.group(4), 16) | ||
# trace sampled is set if the right-most bit in flag component is set | ||
trace_sampled = bool(int_flag & 1) | ||
except (IndexError, AttributeError): | ||
# could not parse header as expected. Return None | ||
pass | ||
return trace_id, span_id, trace_sampled | ||
|
||
|
||
def _parse_xcloud_trace(header): | ||
"""Given an X_CLOUD_TRACE header, extract the trace and span ids. | ||
|
||
Args: | ||
header (str): the string extracted from the X_CLOUD_TRACE header | ||
Returns: | ||
Tuple[Optional[dict], Optional[str]]: | ||
The trace_id and span_id extracted from the header | ||
Tuple[Optional[dict], Optional[str], bool]: | ||
The trace_id, span_id and trace_sampled extracted from the header | ||
Each field will be None if not found. | ||
""" | ||
trace_id = None | ||
span_id = None | ||
trace_id = span_id = None | ||
trace_sampled = False | ||
# see https://cloud.google.com/trace/docs/setup for X-Cloud-Trace_Context format | ||
if header: | ||
try: | ||
split_header = header.split("/", 1) | ||
trace_id = split_header[0] | ||
header_suffix = split_header[1] | ||
# the span is the set of alphanumeric characters after the / | ||
span_id = re.findall(r"^\w+", header_suffix)[0] | ||
regex = r"([\w-]+)?(\/?([\w-]+))?(;?o=(\d))?" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: according to our doc TRACE_ID is 32-character hexadec value that can be presented via acceptible UUID formats. SPAN_ID is a decimal value. adding it because tracing of the w3c was more strict. 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'm kind of curious on your thoughts here. I made the w3c code more strict, because the spec is pretty explicit about dropping invalid headers (which makes sense for an open spec if you want to ensure all implementations are interoperable). I thought we might want to be a bit more permissible in the |
||
match = re.match(regex, header) | ||
trace_id = match.group(1) | ||
span_id = match.group(3) | ||
trace_sampled = match.group(5) == "1" | ||
except IndexError: | ||
pass | ||
return trace_id, span_id | ||
return trace_id, span_id, trace_sampled | ||
|
||
|
||
def get_request_data(): | ||
"""Helper to get http_request and trace data from supported web | ||
frameworks (currently supported: Flask and Django). | ||
|
||
Returns: | ||
Tuple[Optional[dict], Optional[str], Optional[str]]: | ||
Data related to the current http request, trace_id, and span_id for | ||
the request. All fields will be None if a django request isn't | ||
found. | ||
Tuple[Optional[dict], Optional[str], Optional[str], bool]: | ||
Data related to the current http request, trace_id, span_id, and trace_sampled | ||
for the request. All fields will be None if a http request isn't found. | ||
""" | ||
checkers = ( | ||
get_request_data_from_django, | ||
get_request_data_from_flask, | ||
) | ||
|
||
for checker in checkers: | ||
http_request, trace_id, span_id = checker() | ||
http_request, trace_id, span_id, trace_sampled = checker() | ||
if http_request is not None: | ||
return http_request, trace_id, span_id | ||
return http_request, trace_id, span_id, trace_sampled | ||
|
||
return None, None, None | ||
return None, None, None, False |
+2 −1 | deployable/python/snippets.py | |
+36 −0 | tests/common/python.py |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -454,6 +454,7 @@ def test_log_empty(self): | |
|
||
self.assertEqual(len(entries), 1) | ||
self.assertIsNone(entries[0].payload) | ||
self.assertFalse(entries[0].trace_sampled) | ||
|
||
def test_log_struct_logentry_data(self): | ||
logger = Config.CLIENT.logger(self._logger_name("log_w_struct")) | ||
|
@@ -473,6 +474,7 @@ def test_log_struct_logentry_data(self): | |
self.assertEqual(entries[0].severity, "WARNING") | ||
self.assertEqual(entries[0].trace, JSON_PAYLOAD["trace"]) | ||
self.assertEqual(entries[0].span_id, JSON_PAYLOAD["span_id"]) | ||
self.assertFalse(entries[0].trace_sampled) | ||
|
||
def test_log_handler_async(self): | ||
LOG_MESSAGE = "It was the worst of times" | ||
|
@@ -534,6 +536,7 @@ def test_handlers_w_extras(self): | |
extra = { | ||
"trace": "123", | ||
"span_id": "456", | ||
"trace_sampled": True, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: do you want to add a regression test (sampled=false or omitted)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, I added a couple more assertions to this file |
||
"http_request": expected_request, | ||
"source_location": expected_source, | ||
"resource": Resource(type="cloudiot_device", labels={}), | ||
|
@@ -545,6 +548,7 @@ def test_handlers_w_extras(self): | |
self.assertEqual(len(entries), 1) | ||
self.assertEqual(entries[0].trace, extra["trace"]) | ||
self.assertEqual(entries[0].span_id, extra["span_id"]) | ||
self.assertTrue(entries[0].trace_sampled) | ||
self.assertEqual(entries[0].http_request, expected_request) | ||
self.assertEqual( | ||
entries[0].labels, {**extra["labels"], "python_logger": LOGGER_NAME} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since these headers are standardized, is there a need to maintain an instance per web server framework?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for some reason django uses "HTTP_" as a prefix when reading most HTTP headers.
At some point as we add more framework support, we may want to revisit this to add the prefix later in django code instead of in the constant, but for now I'd prefer to leave it as-is