Repository navigation
Wire LOG_LEVEL config into logging.basicConfig - #25
Merged
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Investigating why ckan:resource-update-stress (a NESO UAT performance test)
consistently gets a 404 from datastore_search for resources that genuinely
have data in BigQuery. Traced the 404 to backend.py's _read_schema catching
google.api_core.exceptions.NotFound from BigQuery's own tables.get call —
but the service has no way to show what the real underlying error is,
because LOG_LEVEL was never actually wired up.
Config already declares LOG_LEVEL (core/config.py) and this repo's own
CLAUDE.md roadmap lists "the existing log.debug lines in auth + error
handlers + the CKAN provider light up under LOG_LEVEL=DEBUG" as a stated
goal — but main.py calls logging.basicConfig(level=logging.INFO, ...) with
a hardcoded literal, before the config is even read. Setting the
LOG_LEVEL env var on a deployment currently does nothing.
What changed
datastore/main.py:
logging.basicConfignow readsget_config().LOG_LEVELinstead of a hardcoded
logging.INFO.What was verified
(
pytest tests/ -k "not integration"). Two pre-existing failures intests/test_datastore_dump.py(parquet export column casting) existidentically on
mainwithout this change — confirmed by stashing thediff and re-running each one individually. Everything else passes
(494 passed, same as baseline).
Config.LOG_LEVELis aLiteral["DEBUG","INFO","WARNING","ERROR","CRITICAL"]field with a default of
"INFO", so this change is a no-op for anydeployment that hasn't set
LOG_LEVELexplicitly — behavior onlychanges when the env var is actually set.
Context for the resource-update-stress investigation
Separate from this PR, but the reason it exists: reproduced live with a
fresh sysadmin CKAN token that
datastore_authorizesucceeds (200,correct resource/package) for a resource that then immediately 404s on
datastore_search, and that this persists for days (ruling out aningestion timing race). The only place that 404 can originate in this
codebase is
backend.py's_read_schemacatching a realNotFoundfromBigQuery's
tables.get, even though the same table is trivially queryablewith the same read-only service account from a fresh script. This PR
doesn't fix that bug — it unblocks getting a real error message out of
the service the next time it happens, instead of the generic wrapped 404.
Deployment
Whatever pipeline builds and deploys this service's image needs to pick
up this change before
LOG_LEVEL=DEBUGon a running deployment willactually do anything.