40 add logging aggregation, LOGLEVEL env var, other logging tweaks #45
Conversation
…r reason this prevents the logs from getting blasted when batches full of similar errors present. does not aggregate when the product id appears in the reason
takes an int or string representation of a python standard log level like INFO
alexdunnjpl
requested review from
tloubrieu-jpl,
nutjob4life and
collinss-jpl
as code owners
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
| @@ -92,6 +92,7 @@ | |||
| logging.error(err) | |||
| raise ValueError(f'Failed to parse username/password from PROV_CREDENTIALS value "{provCredentialsStr}": {err}') | |||
|
|
|||
| log_level = parse_log_level(os.environ.get('LOGLEVEL', 'INFO')) | |||
|
|
|||
| def run_factory(sweeper_f: Callable) -> Callable: | |||
There was a problem hiding this comment.
E302: expected 2 blank lines, found 1
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
| Command | Usage |
|---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
| @@ -62,7 +62,7 @@ | |||
| from typing import Callable, Iterable | |||
|
|
|||
| from pds.registrysweepers import provenance, ancestry | |||
| from pds.registrysweepers.utils import configure_logging, get_human_readable_elapsed_since | |||
| from pds.registrysweepers.utils import configure_logging, get_human_readable_elapsed_since, parse_log_level | |||
There was a problem hiding this comment.
reportMissingImports: Import "pds.registrysweepers.utils" could not be resolved
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
| Command | Usage |
|---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
| @@ -143,14 +146,11 @@ def query_registry_db( | |||
| total_hits = data["hits"]["total"]["value"] | |||
| log.debug(f" paging query ({served_hits} to {min(served_hits + page_size, total_hits)} of {total_hits})") | |||
|
|
|||
| last_info_log_at_percentage = 0 | |||
| log.info("Query progress: 0%") | |||
|
|
|||
| for hit in data["hits"]["hits"]: | |||
There was a problem hiding this comment.
there is a tqdm package which can help to track progress in a loop. I would prefer to use that rather than adding specific indicators which can make the code less readable regarding its primary focus.
| @@ -92,6 +92,7 @@ | |||
| logging.error(err) | |||
| raise ValueError(f'Failed to parse username/password from PROV_CREDENTIALS value "{provCredentialsStr}": {err}') | |||
|
|
|||
| log_level = parse_log_level(os.environ.get('LOGLEVEL', 'INFO')) | |||
There was a problem hiding this comment.
I remember that Sean recommended to use a log configuration file to configure the python logs, I don't know how that would integrate in a AWS deployment. We would need to mount the log configuration file in the docker image from a configuration store (I don't know what that is in AWS?). Then we would have the log level in this file.
I don't wnat to overcomplicate the changes right now, so let say it is for comment/discussion, not for change.
There was a problem hiding this comment.
Can you serve config files or complex objects from the Parameter Store? Preferably whatever mechanism we use should be trivially-adjustable in AWS (as environment variables are) to allow us to switch into debug logging easily if an issue is found.
|
@tloubrieu-jpl could I trouble you to open tickets for your two comments? The issues raised relate to longstanding existing code and are out-of-scope for this PR (i.e. I've left it better than I found it and y'all need these fixes yesterday). Happy to pick up these improvements once we're out of the woods on the immediate ops issues. EDIT
Helps if I read the whole thing before firing off a response 😅 |
There was a problem hiding this comment.
I created a ticket for my comments (#47) to be implemented when not in a hurry.
alexdunnjpl
changed the title
🗒️ Summary
Tweaks logging in a handful of ways (see commit messages)
Biggest one is that product-level errors/warns are now aggregated, while maintaining explanatory details for troubleshooting.
Log level can now be set in dockerized processes via the
LOGLEVELenvironment variable⚙️ Test Data and/or Report
Unit tests pass
♻️ Related Issues
fixes #40