Skip to content

feat: make logging API more friendly to use #422

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 19 commits into from
Nov 11, 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
30 changes: 25 additions & 5 deletions google/cloud/logging_v2/logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
("source_location", None),
)

_STRUCT_EXTRACTABLE_FIELDS = ["severity", "trace", "span_id"]


class Logger(object):
"""Loggers represent named targets for log entries.
Expand Down Expand Up @@ -133,6 +135,20 @@ def _do_log(self, client, _entry_class, payload=None, **kw):
kw["labels"] = kw.pop("labels", self.labels)
kw["resource"] = kw.pop("resource", self.default_resource)

severity = kw.get("severity", None)
if isinstance(severity, str) and not severity.isupper():
# convert severity to upper case, as expected by enum definition
kw["severity"] = severity.upper()

if isinstance(kw["resource"], collections.abc.Mapping):
# if resource was passed as a dict, attempt to parse it into a
# Resource object
try:
kw["resource"] = Resource(**kw["resource"])
except TypeError as e:
# dict couldn't be parsed as a Resource
raise TypeError("invalid resource dict") from e
Copy link
Contributor

Choose a reason for hiding this comment

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

does it mean the writing log fails? is it a current user experience in the case of other failures?

Choose a reason for hiding this comment

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

+1 to the question. We generally should try to avoid crashing the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this change, the library would already throw the same exception here for the same inputs. This change attempts to turn certain error-raising inputs into valid inputs. But some inputs will still be invalid

Also, note that exceptions are used for control flow in Python, so raising an exception doesn't necessarily mean "crash"


if payload is not None:
entry = _entry_class(payload=payload, **kw)
else:
Expand Down Expand Up @@ -186,6 +202,10 @@ def log_struct(self, info, *, client=None, **kw):
kw (Optional[dict]): additional keyword arguments for the entry.
See :class:`~logging_v2.entries.LogEntry`.
"""
for field in _STRUCT_EXTRACTABLE_FIELDS:
# attempt to copy relevant fields from the payload into the LogEntry body
if field in info and field not in kw:
kw[field] = info[field]
self._do_log(client, StructEntry, info, **kw)

def log_proto(self, message, *, client=None, **kw):
Expand Down Expand Up @@ -220,14 +240,14 @@ def log(self, message=None, *, client=None, **kw):
kw (Optional[dict]): additional keyword arguments for the entry.
See :class:`~logging_v2.entries.LogEntry`.
"""
entry_type = LogEntry
if isinstance(message, google.protobuf.message.Message):
entry_type = ProtobufEntry
self.log_proto(message, client=client, **kw)
elif isinstance(message, collections.abc.Mapping):
entry_type = StructEntry
self.log_struct(message, client=client, **kw)
elif isinstance(message, str):
entry_type = TextEntry
self._do_log(client, entry_type, message, **kw)
self.log_text(message, client=client, **kw)
else:
self._do_log(client, LogEntry, message, **kw)

def delete(self, logger_name=None, *, client=None):
"""Delete all entries in a logger via a DELETE request
Expand Down
19 changes: 19 additions & 0 deletions tests/system/test_system.py
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,25 @@ def test_log_empty(self):
self.assertEqual(len(entries), 1)
self.assertIsNone(entries[0].payload)

def test_log_struct_logentry_data(self):
logger = Config.CLIENT.logger(self._logger_name("log_w_struct"))
self.to_delete.append(logger)

JSON_PAYLOAD = {
"message": "System test: test_log_struct_logentry_data",
"severity": "warning",
"trace": "123",
"span_id": "456",
}
logger.log(JSON_PAYLOAD)
entries = _list_entries(logger)

self.assertEqual(len(entries), 1)
self.assertEqual(entries[0].payload, JSON_PAYLOAD)
self.assertEqual(entries[0].severity, "WARNING")
self.assertEqual(entries[0].trace, JSON_PAYLOAD["trace"])
self.assertEqual(entries[0].span_id, JSON_PAYLOAD["span_id"])

def test_log_handler_async(self):
LOG_MESSAGE = "It was the worst of times"

Expand Down
101 changes: 101 additions & 0 deletions tests/unit/test_logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,107 @@ def test_log_struct_w_explicit(self):

self.assertEqual(api._write_entries_called_with, (ENTRIES, None, None, None))

def test_log_struct_inference(self):
"""
LogEntry fields in _STRUCT_EXTRACTABLE_FIELDS should be inferred from
the payload data if not passed as a parameter
"""
from google.cloud.logging_v2.handlers._monitored_resources import (
detect_resource,
)

STRUCT = {
"message": "System test: test_log_struct_logentry_data",
"severity": "warning",
"trace": "123",
"span_id": "456",
}
RESOURCE = detect_resource(self.PROJECT)._to_dict()
ENTRIES = [
{
"logName": "projects/%s/logs/%s" % (self.PROJECT, self.LOGGER_NAME),
"jsonPayload": STRUCT,
"severity": "WARNING",
"trace": "123",
"spanId": "456",
"resource": RESOURCE,
}
]
client = _Client(self.PROJECT)
api = client.logging_api = _DummyLoggingAPI()
logger = self._make_one(self.LOGGER_NAME, client=client)

logger.log_struct(STRUCT, resource=RESOURCE)

self.assertEqual(api._write_entries_called_with, (ENTRIES, None, None, None))

def test_log_w_dict_resource(self):
"""
Users should be able to input a dictionary with type and labels instead
of a Resource object
"""
import pytest

MESSAGE = "hello world"
client = _Client(self.PROJECT)
api = client.logging_api = _DummyLoggingAPI()
logger = self._make_one(self.LOGGER_NAME, client=client)
broken_resource_dicts = [{}, {"type": ""}, {"labels": ""}]
for resource in broken_resource_dicts:
# ensure bad inputs result in a helpful error
with pytest.raises(TypeError):
logger.log(MESSAGE, resource=resource)
# ensure well-formed dict is converted to a resource
resource = {"type": "gae_app", "labels": []}
ENTRIES = [
{
"logName": "projects/%s/logs/%s" % (self.PROJECT, self.LOGGER_NAME),
"textPayload": MESSAGE,
"resource": resource,
}
]
logger.log(MESSAGE, resource=resource)
self.assertEqual(api._write_entries_called_with, (ENTRIES, None, None, None))

def test_log_lowercase_severity(self):
"""
lower case severity strings should be accepted
"""
from google.cloud.logging_v2.handlers._monitored_resources import (
detect_resource,
)

for lower_severity in [
"default",
"debug",
"info",
"notice",
"warning",
"error",
"critical",
"alert",
"emergency",
]:
MESSAGE = "hello world"
RESOURCE = detect_resource(self.PROJECT)._to_dict()
ENTRIES = [
{
"logName": "projects/%s/logs/%s" % (self.PROJECT, self.LOGGER_NAME),
"textPayload": MESSAGE,
"resource": RESOURCE,
"severity": lower_severity.upper(),
}
]
client = _Client(self.PROJECT)
api = client.logging_api = _DummyLoggingAPI()
logger = self._make_one(self.LOGGER_NAME, client=client)

logger.log(MESSAGE, severity=lower_severity)

self.assertEqual(
api._write_entries_called_with, (ENTRIES, None, None, None)
)

def test_log_proto_defaults(self):
from google.cloud.logging_v2.handlers._monitored_resources import (
detect_resource,
Expand Down