Skip to content

fix: allow reading logs from non-project paths #444

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 4 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
17 changes: 12 additions & 5 deletions google/cloud/logging_v2/entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@
)


def logger_name_from_path(path):
def logger_name_from_path(path, project=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Perhaps I am not well familiar with Python, but is there a way to create another logger_name_from_path function with project parameter? I think that it would be better then having parameters with default values

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 think this is the Pythonic way to do it. Especially since this function is just directly calling another function. If it wasn't a public function already in use, I would prefer to remove this one altogether

What's the argument against default parameters?

"""Validate a logger URI path and get the logger name.

Args:
path (str): URI path for a logger API request
project (str): The project the path is expected to belong to

Returns:
str: Logger name parsed from ``path``.
Expand All @@ -59,7 +60,7 @@ def logger_name_from_path(path):
ValueError: If the ``path`` is ill-formed of if the project
from ``path`` does not agree with the ``project`` passed in.
"""
return _name_from_project_path(path, None, _LOGGER_TEMPLATE)
return _name_from_project_path(path, project, _LOGGER_TEMPLATE)


def _int_or_none(value):
Expand Down Expand Up @@ -155,7 +156,8 @@ def from_api_repr(cls, resource, client, *, loggers=None):
Client which holds credentials and project configuration.
loggers (Optional[dict]):
A mapping of logger fullnames -> loggers. If not
passed, the entry will have a newly-created logger.
passed, the entry will have a newly-created logger if possible,
or an empty logger field if not.

Returns:
google.cloud.logging.entries.LogEntry: Log entry parsed from ``resource``.
Expand All @@ -165,8 +167,13 @@ def from_api_repr(cls, resource, client, *, loggers=None):
logger_fullname = resource["logName"]
logger = loggers.get(logger_fullname)
if logger is None:
logger_name = logger_name_from_path(logger_fullname)
logger = loggers[logger_fullname] = client.logger(logger_name)
# attempt to create a logger if possible
try:
logger_name = logger_name_from_path(logger_fullname, client.project)
logger = loggers[logger_fullname] = client.logger(logger_name)
except ValueError:
# log name is not scoped to a project. Leave logger as None
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment true? Seems that logger is initialized with value returned from loggers.get(logger_fullname) call...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

loggers.get(logger_fullname) will return None if the loggers dict doesn't contain logger_fullname

If it's None. Lines 171 to 173 will try to contruct a new logger associated with logger_name, which will work if logger_name is a project id. If it's an organization or a folder, it will throw an exception. In that case, we can just leave the logger as empty and move on

pass
payload = cls._extract_payload(resource)
insert_id = resource.get("insertId")
timestamp = resource.get("timestamp")
Expand Down
84 changes: 82 additions & 2 deletions tests/unit/test_entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@


class Test_logger_name_from_path(unittest.TestCase):
def _call_fut(self, path):
def _call_fut(self, path, project=None):
from google.cloud.logging_v2.entries import logger_name_from_path

return logger_name_from_path(path)
return logger_name_from_path(path, project)

def test_w_simple_name(self):
LOGGER_NAME = "LOGGER_NAME"
Expand All @@ -37,6 +37,30 @@ def test_w_name_w_all_extras(self):
logger_name = self._call_fut(PATH)
self.assertEqual(logger_name, LOGGER_NAME)

def test_w_wrong_project(self):
LOGGER_NAME = "LOGGER_NAME"
IN_PROJECT = "in-project"
PATH_PROJECT = "path-project"
PATH = "projects/%s/logs/%s" % (PATH_PROJECT, LOGGER_NAME)
with self.assertRaises(ValueError):
self._call_fut(PATH, IN_PROJECT)

def test_invalid_inputs(self):
invalid_list = [
"",
"abc/123/logs/456",
"projects//logs/",
"projects/123/logs",
"projects/123logs/",
"projects123/logs",
"project/123",
"projects123logs456",
"/logs/123",
]
for path in invalid_list:
with self.assertRaises(ValueError):
self._call_fut(path)


class Test__int_or_none(unittest.TestCase):
def _call_fut(self, value):
Expand Down Expand Up @@ -315,6 +339,62 @@ def test_from_api_repr_w_loggers_w_logger_match(self):
self.assertEqual(entry.operation, OPERATION)
self.assertIsNone(entry.payload)

def test_from_api_repr_w_folder_path(self):
from datetime import datetime
from datetime import timedelta
from google.cloud._helpers import UTC

client = _Client(self.PROJECT)
IID = "IID"
NOW = datetime.utcnow().replace(tzinfo=UTC)
LATER = NOW + timedelta(seconds=1)
TIMESTAMP = _datetime_to_rfc3339_w_nanos(NOW)
RECEIVED = _datetime_to_rfc3339_w_nanos(LATER)
LOG_NAME = "folders/%s/logs/%s" % (self.PROJECT, self.LOGGER_NAME)
LABELS = {"foo": "bar", "baz": "qux"}
TRACE = "12345678-1234-5678-1234-567812345678"
SPANID = "000000000000004a"
FILE = "my_file.py"
LINE_NO = 123
FUNCTION = "my_function"
SOURCE_LOCATION = {"file": FILE, "line": str(LINE_NO), "function": FUNCTION}
OP_ID = "OP_ID"
PRODUCER = "PRODUCER"
OPERATION = {"id": OP_ID, "producer": PRODUCER, "first": True, "last": False}
API_REPR = {
"logName": LOG_NAME,
"insertId": IID,
"timestamp": TIMESTAMP,
"receiveTimestamp": RECEIVED,
"labels": LABELS,
"trace": TRACE,
"spanId": SPANID,
"traceSampled": True,
"sourceLocation": SOURCE_LOCATION,
"operation": OPERATION,
}
klass = self._get_target_class()

entry = klass.from_api_repr(API_REPR, client)

self.assertEqual(entry.log_name, LOG_NAME)
self.assertIsNone(entry.logger)
self.assertEqual(entry.insert_id, IID)
self.assertEqual(entry.timestamp, NOW)
self.assertEqual(entry.received_timestamp, LATER)
self.assertEqual(entry.labels, LABELS)
self.assertEqual(entry.trace, TRACE)
self.assertEqual(entry.span_id, SPANID)
self.assertTrue(entry.trace_sampled)

source_location = entry.source_location
self.assertEqual(source_location["file"], FILE)
self.assertEqual(source_location["line"], LINE_NO)
self.assertEqual(source_location["function"], FUNCTION)

self.assertEqual(entry.operation, OPERATION)
self.assertIsNone(entry.payload)

def test_to_api_repr_w_source_location_no_line(self):
from google.cloud.logging_v2.logger import _GLOBAL_RESOURCE

Expand Down
30 changes: 30 additions & 0 deletions tests/unit/test_logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,36 @@ def test_list_entries_limit(self):
},
)

def test_list_entries_folder(self):
from google.cloud.logging import TextEntry
from google.cloud.logging import Client

client = Client(
project=self.PROJECT, credentials=_make_credentials(), _use_grpc=False
)
FOLDER_ID = "123"
LOG_NAME = f"folders/{FOLDER_ID}/logs/cloudaudit.googleapis.com%2Fdata_access"

ENTRIES = [
{
"textPayload": "hello world",
"insertId": "1",
"resource": {"type": "global"},
"logName": LOG_NAME,
},
]
returned = {"entries": ENTRIES}
client._connection = _Connection(returned)

iterator = client.list_entries(resource_names=[f"folder/{FOLDER_ID}"],)
entries = list(iterator)
# Check the entries.
self.assertEqual(len(entries), 1)
entry = entries[0]
self.assertIsInstance(entry, TextEntry)
self.assertIsNone(entry.logger)
self.assertEqual(entry.log_name, LOG_NAME)


class TestBatch(unittest.TestCase):

Expand Down