feat: support http_request field #120
Conversation
| header = flask.request.headers.get(_FLASK_TRACE_HEADER) | ||
| if header: | ||
| trace_id = header.split("/", 1)[0] |
There was a problem hiding this comment.
Future edge case to take care of:
Given user input is X-Cloud-Trace-Context: TRACE_ID/SPAN_ID;o=TRACE_TRUE where any of the values can be nil or 0. This logic to split by / and grab the first value might give you span id instead.
Seems super niche edge case, but I actually got a similar ticket for this in Go.
There was a problem hiding this comment.
Hmm I'm not sure I understand. Can you have a span without a trace? Do you have a ticket or doc I can look over?
Either way, I'm not a huge fan of this parsing logic. I'm hoping to fix it up when I add support for spans soon
There was a problem hiding this comment.
It shouldn't be possible to have a trace as null but span as non-null. Curious how this was a case in Go?
There was a problem hiding this comment.
The edge cases I'm referring to here is just that the X-Cloud-Trace-Context field is provided by the user.
So the user could manually write anything here. e.g. '/hello/world' and we'd grep the first token as trace_id. Adding better regex later on could help.
This was the original Go issue.
There was a problem hiding this comment.
Where we tell users they can manually write this header: https://cloud.google.com/trace/docs/setup#force-trace
There was a problem hiding this comment.
Hmm good to keep in mind, yeah it might be helpful to throw an exception or something if the header doesn't match the expected format
I'll probably fix that in a later PR when I add support for spans though. That's when proper parsing will be more important. For now, this is the logic that was already in place
| header = flask.request.headers.get(_FLASK_TRACE_HEADER) | ||
| if header: | ||
| trace_id = header.split("/", 1)[0] |
There was a problem hiding this comment.
It shouldn't be possible to have a trace as null but span as non-null. Curious how this was a case in Go?
| flask_expected = (None, _FLASK_TRACE_ID) | ||
| django_expected = (None, _DJANGO_TRACE_ID) |
There was a problem hiding this comment.
perhaps, worth splitting this into two separate tests - for django and for flask
There was a problem hiding this comment.
This test specifically is focused on the integration function logic that grabs from either django or flask. The django and flask components are mocked out here
e462b5f to
6d1974d
Compare
google/cloud/logging_v2/handlers/transports/background_thread.py
Outdated
Show resolved
Hide resolved
google/cloud/logging_v2/handlers/transports/background_thread.py
Outdated
Show resolved
Hide resolved
| resource=self.resource, | ||
| labels=gae_labels, | ||
| trace=trace_id, | ||
| http_request=http_request, |
There was a problem hiding this comment.
I see you're using http_pb2_request does that work properly with the LogEntry?
There was a problem hiding this comment.
I believe it should. This is the type used by the underlying LogEntry proto, so I assumed it would be preferred:
But I'm still getting used to working with gapic code. Do you prefer to just use dictionaries for these kinds of fields?
This PR adds support for the
http_requestfield that's part of the Cloud Logging LogEntry spec. Details about the request (method, url, payload size, user agent, remote ip, referrer) will be attached as metadata to each log when possible. These fields will be inferred from Flask and Django environments, just as thetrace_idcurrently is.I plan to allow users to set this information manually as well for users using unsupported libraries, but that will come in a separate PR
Currently only fully supported on AppEngine
This fixes #72, partially addresses #110, and lays some groundwork for later improvements