-
Notifications
You must be signed in to change notification settings - Fork 55
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,11 +46,12 @@ | |
) | ||
|
||
|
||
def logger_name_from_path(path): | ||
def logger_name_from_path(path, project=None): | ||
"""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``. | ||
|
@@ -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): | ||
|
@@ -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``. | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If it's |
||
pass | ||
payload = cls._extract_payload(resource) | ||
insert_id = resource.get("insertId") | ||
timestamp = resource.get("timestamp") | ||
|
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: Perhaps I am not well familiar with Python, but is there a way to create another
logger_name_from_path
function withproject
parameter? I think that it would be better then having parameters with default valuesThere 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 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?