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

fix: improve API compatibility for next release #292

Merged
merged 12 commits into from May 12, 2021

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented May 11, 2021

  • As aprt of the next release, I removed some fields in the http_request data (referer, remoteIp, requestSize), because they don't work consistently across all GCP environments yet. I realized this would be considered a breaking change for AppEngine users who may already be relying on these fields. Instead of removing the fields, non-appengine handlers now filter them out. We can re-consider the future of these fields for v3.0.0
  • StructuredLogHandler formatting breaks if users log text with quotes in it. Now, the handler will add "" before each quote found in the message string
  • The Flask request detection broke in the unit tests, so I also fixed that

@daniel-sanche daniel-sanche self-assigned this May 11, 2021
@daniel-sanche daniel-sanche requested review from as code owners May 11, 2021
@product-auto-label product-auto-label bot added the api: logging label May 11, 2021
@daniel-sanche daniel-sanche added the kokoro:run label May 11, 2021
@google-cla google-cla bot added the cla: yes label May 11, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run label May 11, 2021
@@ -45,6 +45,10 @@ class CloudLoggingFilter(logging.Filter):
overwritten using the `extras` argument when writing logs.
"""

# The subset of http_request fields have been tested to work consistently across GCP environments
Copy link
Contributor

@0xSage 0xSage May 12, 2021

Choose a reason for hiding this comment

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

Great commenting throughout this PR.

import json

handler = self._make_one()
message = '"test"'
Copy link
Contributor

@0xSage 0xSage May 12, 2021

Choose a reason for hiding this comment

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

Are other special chars escaped as well?

Copy link
Contributor Author

@daniel-sanche daniel-sanche May 12, 2021

Choose a reason for hiding this comment

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

I haven't seen any that need it. Let me know if you find others, I imagine it will be the same across languages

0xSage
0xSage approved these changes May 12, 2021
@daniel-sanche daniel-sanche added the kokoro:force-run label May 12, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label May 12, 2021
@daniel-sanche daniel-sanche merged commit dfe916b into v2_update_2 May 12, 2021
17 checks passed
@daniel-sanche daniel-sanche deleted the extra-http-fields branch May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants