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: add json_fields extras argument for adding to jsonPayload #447

Merged
merged 6 commits into from
Dec 7, 2021

Conversation

daniel-sanche
Copy link
Contributor

v3.0.0 will support sending jsonPayload log entries by logging a dict object: cloud_logger.error(my_dict). This feature is technically counter to the python logging spec though, which expects string inputs. For full compatibility, we advise users to send their dicts encoded as JSON like this: cloud_logger.error(json.dumps(my_dict))

These methods of sending JSON payloads don't work for all customers though, so this PR adds a third method: users can now send json data through the extras argument: cloud_logger.error('bad news', extra={json_fields: my_dict})). Logging this way will result in a jsonPayload log, made up of the json_fields dict combined with the log message.

Fixes #411

@daniel-sanche daniel-sanche requested review from a team as code owners November 16, 2021 23:53
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2021
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/python-logging API. label Nov 16, 2021
Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

lgtm. i did not do in-depth analysis of system and unit tests. maybe someone will look into it latter..

return ContainerEngineHandler(**kw)
return StructuredLogHandler(**kw, project_id=self.project)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think that I saw this commit in PR #450. Is it possible there had to be a rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it seems like there was a merging issue in the v3.0.0 branch at some point which changed this line, and I had to fix it in both branches for tests to pass

Comment on lines +228 to +229
if passed_json_fields and isinstance(
passed_json_fields, collections.abc.Mapping
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if passed_json_fields is undefined (i.e. if not passed_json_fields then the line 226 probably will fail. should we check for it sooner?

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'm not sure I understand. Line 226 should be unrelated to passed_json_fields. It's checking if the msg field was populated by a dictionary. If so, lines 228-232 will attempt to add the additional passed_json_fields fields to the msg dictionary (if present)

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 tried to improve the comments here

Comment on lines +333 to +334
message = "name: %s"
name_arg = "Daniel"
Copy link
Contributor

Choose a reason for hiding this comment

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

question: i am not familiar with the interface, is it supposed to format the message like with print() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that Python's logging std lib allows you to build formated log strings like this: logging.info("my name is: %s", "Daniel"). This test just makes sure that our GCP library additions don't break that expected functionality

Copy link
Contributor

@losalex losalex left a comment

Choose a reason for hiding this comment

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

LGTM

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants