-
Notifications
You must be signed in to change notification settings - Fork 55
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
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
if passed_json_fields and isinstance( | ||
passed_json_fields, collections.abc.Mapping |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
message = "name: %s" | ||
name_arg = "Daniel" |
There was a problem hiding this comment.
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() ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 thejson_fields
dict combined with the log message.Fixes #411