Skip to content

feat: add json_fields extras argument for adding to jsonPayload #447

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

Merged
merged 6 commits into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion google/cloud/logging_v2/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ def get_default_handler(self, **kw):
if monitored_resource.type == _GAE_RESOURCE_TYPE:
return CloudLoggingHandler(self, resource=monitored_resource, **kw)
elif monitored_resource.type == _GKE_RESOURCE_TYPE:
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

elif monitored_resource.type == _GCF_RESOURCE_TYPE:
# __stdout__ stream required to support structured logging on Python 3.7
kw["stream"] = kw.get("stream", sys.__stdout__)
Expand Down
16 changes: 14 additions & 2 deletions google/cloud/logging_v2/handlers/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,16 @@ def _format_and_parse_message(record, formatter_handler):
record (logging.LogRecord): The record object representing the log
formatter_handler (logging.Handler): The handler used to format the log
"""
# if message is a dictionary, return as-is
passed_json_fields = getattr(record, "json_fields", {})
# if message is a dictionary, use dictionary directly
if isinstance(record.msg, collections.abc.Mapping):
return record.msg
payload = record.msg
# attach any extra json fields if present
if passed_json_fields and isinstance(
passed_json_fields, collections.abc.Mapping
Comment on lines +229 to +230
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

):
payload = {**payload, **passed_json_fields}
return payload
# format message string based on superclass
message = formatter_handler.format(record)
try:
Expand All @@ -235,6 +242,11 @@ def _format_and_parse_message(record, formatter_handler):
except (json.decoder.JSONDecodeError, IndexError):
# log string is not valid json
pass
# if json_fields was set, create a dictionary using that
if passed_json_fields and isinstance(passed_json_fields, collections.abc.Mapping):
if message != "None":
passed_json_fields["message"] = message
return passed_json_fields
# if formatted message contains no content, return None
return message if message != "None" else None

Expand Down
25 changes: 25 additions & 0 deletions tests/system/test_system.py
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,31 @@ def test_handlers_w_extras(self):
)
self.assertEqual(entries[0].resource.type, extra["resource"].type)

def test_handlers_w_json_fields(self):
LOG_MESSAGE = "Testing with json_field extras."
LOGGER_NAME = "json_field_extras"
handler_name = self._logger_name(LOGGER_NAME)

handler = CloudLoggingHandler(
Config.CLIENT, name=handler_name, transport=SyncTransport
)

# only create the logger to delete, hidden otherwise
logger = Config.CLIENT.logger(handler.name)
self.to_delete.append(logger)

cloud_logger = logging.getLogger(LOGGER_NAME)
cloud_logger.addHandler(handler)
extra = {"json_fields": {"hello": "world", "two": 2}}
cloud_logger.warn(LOG_MESSAGE, extra=extra)

entries = _list_entries(logger)
self.assertEqual(len(entries), 1)
payload = entries[0].payload
self.assertEqual(payload["message"], LOG_MESSAGE)
self.assertEqual(payload["hello"], "world")
self.assertEqual(payload["two"], 2)

def test_log_root_handler(self):
LOG_MESSAGE = "It was the best of times."

Expand Down
90 changes: 90 additions & 0 deletions tests/unit/handlers/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,40 @@ def test_emit_dict(self):
),
)

def test_emit_w_json_extras(self):
"""
User can add json_fields to the record, which should populate the payload
"""
from google.cloud.logging_v2.logger import _GLOBAL_RESOURCE

client = _Client(self.PROJECT)
handler = self._make_one(
client, transport=_Transport, resource=_GLOBAL_RESOURCE,
)
message = "message"
json_fields = {"hello": "world"}
logname = "logname"
expected_label = {"python_logger": logname}
record = logging.LogRecord(
logname, logging.INFO, None, None, message, None, None
)
setattr(record, "json_fields", json_fields)
handler.handle(record)

self.assertEqual(
handler.transport.send_called_with,
(
record,
{"message": "message", "hello": "world"},
_GLOBAL_RESOURCE,
expected_label,
None,
None,
None,
None,
),
)

def test_emit_with_encoded_json(self):
"""
Handler should parse json encoded as a string
Expand Down Expand Up @@ -608,6 +642,62 @@ def test_broken_encoded_dict(self):
result = _format_and_parse_message(record, handler)
self.assertEqual(result, message)

def test_json_fields(self):
"""
record.json_fields should populate the json payload
"""
from google.cloud.logging_v2.handlers.handlers import _format_and_parse_message

message = "hello"
json_fields = {"key": "val"}
record = logging.LogRecord("logname", None, None, None, message, None, None)
setattr(record, "json_fields", json_fields)
handler = logging.StreamHandler()
result = _format_and_parse_message(record, handler)
self.assertEqual(result, {"message": message, "key": "val"})

def test_empty_json_fields(self):
"""
empty jsond_field dictionaries should result in a string output
"""
from google.cloud.logging_v2.handlers.handlers import _format_and_parse_message

message = "hello"
record = logging.LogRecord("logname", None, None, None, message, None, None)
setattr(record, "json_fields", {})
handler = logging.StreamHandler()
result = _format_and_parse_message(record, handler)
self.assertEqual(result, message)

def test_json_fields_empty_message(self):
"""
empty message fields should not be added to json_fields dictionaries
"""
from google.cloud.logging_v2.handlers.handlers import _format_and_parse_message

message = None
json_fields = {"key": "val"}
record = logging.LogRecord("logname", None, None, None, message, None, None)
setattr(record, "json_fields", json_fields)
handler = logging.StreamHandler()
result = _format_and_parse_message(record, handler)
self.assertEqual(result, json_fields)

def test_json_fields_with_json_message(self):
"""
if json_fields and message are both dicts, they should be combined
"""
from google.cloud.logging_v2.handlers.handlers import _format_and_parse_message

message = {"key_m": "val_m"}
json_fields = {"key_j": "val_j"}
record = logging.LogRecord("logname", None, None, None, message, None, None)
setattr(record, "json_fields", json_fields)
handler = logging.StreamHandler()
result = _format_and_parse_message(record, handler)
self.assertEqual(result["key_m"], message["key_m"])
self.assertEqual(result["key_j"], json_fields["key_j"])


class TestSetupLogging(unittest.TestCase):
def _call_fut(self, handler, excludes=None):
Expand Down
23 changes: 23 additions & 0 deletions tests/unit/handlers/test_structured_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -321,3 +321,26 @@ def test_format_overrides(self):
result = json.loads(handler.format(record))
for (key, value) in expected_payload.items():
self.assertEqual(value, result[key])

def test_format_with_json_fields(self):
"""
User can add json_fields to the record, which should populate the payload
"""
import logging
import json

handler = self._make_one()
message = "name: %s"
name_arg = "Daniel"
Comment on lines +333 to +334
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

expected_result = "name: Daniel"
json_fields = {"hello": "world", "number": 12}
record = logging.LogRecord(
None, logging.INFO, None, None, message, name_arg, None,
)
record.created = None
setattr(record, "json_fields", json_fields)
handler.filter(record)
result = json.loads(handler.format(record))
self.assertEqual(result["message"], expected_result)
self.assertEqual(result["hello"], "world")
self.assertEqual(result["number"], 12)