Skip to content
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

feat: support http_request field #120

Merged
merged 27 commits into from Dec 16, 2020
Merged

Conversation

daniel-sanche
Copy link
Contributor

This PR adds support for the http_request field 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 the trace_id currently 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

@daniel-sanche daniel-sanche requested review from a team as code owners December 10, 2020 01:14
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/python-logging API. label Dec 10, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 10, 2020
@simonz130 simonz130 added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Dec 10, 2020
header = flask.request.headers.get(_FLASK_TRACE_HEADER)
if header:
trace_id = header.split("/", 1)[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be possible to have a trace as null but span as non-null. Curious how this was a case in Go?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where we tell users they can manually write this header: https://cloud.google.com/trace/docs/setup#force-trace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be possible to have a trace as null but span as non-null. Curious how this was a case in Go?

google/cloud/logging_v2/handlers/_helpers.py Outdated Show resolved Hide resolved
google/cloud/logging_v2/handlers/_helpers.py Outdated Show resolved Hide resolved
tests/unit/handlers/test__helpers.py Outdated Show resolved Hide resolved
tests/unit/handlers/test__helpers.py Outdated Show resolved Hide resolved
Comment on lines +254 to +255
flask_expected = (None, _FLASK_TRACE_ID)
django_expected = (None, _DJANGO_TRACE_ID)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps, worth splitting this into two separate tests - for django and for flask

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tests/unit/handlers/test_app_engine.py Show resolved Hide resolved
resource=self.resource,
labels=gae_labels,
trace=trace_id,
http_request=http_request,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you're using http_pb2_request does that work properly with the LogEntry?

http_request = resource.get("httpRequest")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it should. This is the type used by the underlying LogEntry proto, so I assumed it would be preferred:

http_request = proto.Field(

But I'm still getting used to working with gapic code. Do you prefer to just use dictionaries for these kinds of fields?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/python-logging API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Flask integration
5 participants