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

feat: support http_request field #120

merged 27 commits into from Dec 16, 2020

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Dec 10, 2020

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 as code owners Dec 10, 2020
@product-auto-label product-auto-label bot added the api: logging label Dec 10, 2020
@google-cla google-cla bot added the cla: yes label Dec 10, 2020
@simonz130 simonz130 added the type: feature request label Dec 10, 2020
header = flask.request.headers.get(_FLASK_TRACE_HEADER)
if header:
trace_id = header.split("/", 1)[0]
Copy link
Contributor

@0xSage 0xSage Dec 10, 2020

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

@daniel-sanche daniel-sanche Dec 10, 2020

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

@0xSage 0xSage Dec 14, 2020

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

@0xSage 0xSage Dec 14, 2020

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

@daniel-sanche daniel-sanche Dec 14, 2020

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
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

@daniel-sanche daniel-sanche Dec 11, 2020

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
google/cloud/logging_v2/handlers/_helpers.py Outdated Show resolved Hide resolved
google/cloud/logging_v2/handlers/transports/base.py Outdated Show resolved Hide resolved
google/cloud/logging_v2/handlers/transports/sync.py Outdated Show resolved Hide resolved
resource=self.resource,
labels=gae_labels,
trace=trace_id,
http_request=http_request,
Copy link
Collaborator

@busunkim96 busunkim96 Dec 15, 2020

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

@daniel-sanche daniel-sanche Dec 15, 2020

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?

@daniel-sanche daniel-sanche merged commit ba94afb into master Dec 16, 2020
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging cla: yes type: feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants