Added logging to src/pelicanfs/core.py #69
Conversation
There was a problem hiding this comment.
Most of the new debug statements seem a bit brief/terse, and some only really log the name of the function they're in. For instance, the two messages in _check_fspath() are currently "Checking path: {path}" and "Compatible path: {path}." If these log statements are primarily intended for developers, they provide enough information to locate sections in the codebase via grep. However, if they are also meant to assist non-technical users or provide more context in logs, they might benefit from being more descriptive. For example, "Checking path {path} for compatibility with blah blah blah" would add clarity on what's being checked and why.
| if fileloc[0] == "/": | ||
| fileloc = fileloc[1:] | ||
|
|
||
| if not self.directorUrl: | ||
| logger.debug("Director URL not set, geting from disocyr url") |
| @@ -220,6 +223,7 @@ async def get_director_headers(self, fileloc, origin=False) -> dict[str, str]: | |||
| directorUrl = directorUrl + "/" | |||
| self.directorUrl = directorUrl | |||
|
|
|||
| logger.debug(f"Getting headers from director: {self.directorUrl}") | |||
There was a problem hiding this comment.
This debug message gets printed twice with different phrasing
| @@ -232,15 +236,18 @@ async def get_working_cache(self, fileloc: str) -> str: | |||
| """ | |||
| Returns the highest priority cache for the namespace that appears to be working | |||
| """ | |||
| logger.debug(f"Get a working cache for {fileloc}") | |||
There was a problem hiding this comment.
The verbiage here feels wrong. Maybe "choosing a cache for ..."?
| return cacheUrl | ||
|
|
||
| # Calculate the list of applicable caches; this takes into account the | ||
| # preferredCaches for the filesystem. If '+' is a preferred cache, we | ||
| # add all the director-provided caches to the list (doing a round of de-dup) | ||
| logger.debug("Getting a cache from the director") |
There was a problem hiding this comment.
This one is also logged twice
| session.headers["Authorization"] = self.token | ||
| try: | ||
| logger.debug(f"Testing cacheurl {updatedUrl}") |
There was a problem hiding this comment.
cacheurl --> cache url
It might also be nice to include some information about what's being tested, e.g. f"testing cache {updatedUrl} for blah blah blah"
| namespace_info = self._get_prefix_info(fileloc) | ||
| if not namespace_info: | ||
| return | ||
|
|
||
|
|
||
| logger.debug(f"Match found") |
There was a problem hiding this comment.
You can also log the match here
| @@ -342,10 +353,12 @@ def _get_prefix_info(self, path: str) -> _CacheManager: | |||
| return namespace_info | |||
|
|
|||
| def _match_namespace(self, fileloc: str): | |||
| logger.debug(f"Matching namespace for {fileloc}") | |||
There was a problem hiding this comment.
Maybe "Extracting namespace prefix from object {fileloc}"? I don't find "match" very descriptive of what's going on here
| @@ -535,6 +555,7 @@ async def open_async(self, path, **kwargs): | |||
| data_url = await self.get_origin_url(path) | |||
| else: | |||
| data_url = self.get_working_cache(path) | |||
| logger.debug(f"Running open_aync on {data_url}") | |||
| @@ -511,6 +528,7 @@ def _check_fspath(self, path: str) -> str: | |||
| check that the pelican://-style URL is compatible with the current | |||
| filesystem object and return the path. | |||
| """ | |||
| logger.debug(f"Checking path: {path}") | |||
There was a problem hiding this comment.
Will someone seeing this in a log know what "checking path" means? Or is it worth being more verbose about what's being checked?
| @@ -402,6 +417,7 @@ async def _glob(self, path, maxdepth=None, **kwargs): | |||
| except it cleans the path url of double '//' and checks for | |||
| the dirlisthost ahead of time | |||
| """ | |||
| logger.debug("Starting glob...") | |||
There was a problem hiding this comment.
This is the only spot you use an ellipsis in these types of debug statements. I actually like this -- to me it signifies the beginning of some multi-step process. Either way, can you pick one or the other and use it throughout?
close #58