Repository navigation
feat: add debugging to feed download process #1861
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
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
3c5bb14
add verifier script and endpoints. add debug info to failing http req…
davidgamez 2aaaaab
Merge branch 'main' into feat_add_download_debbug_info
davidgamez baf7845
Merge branch 'main' into feat_add_download_debbug_info
davidgamez 238db6f
simplify local script
davidgamez 911227d
add missing open api schema endpoint
davidgamez 616cb65
Merge branch 'main' into feat_add_download_debbug_info
davidgamez f4630d3
remove limits to the debug download feature
davidgamez 8e81a3a
add option to run nightly job on a feeds with ops API
davidgamez 64bd12d
fix lastest hash issue
davidgamez 58ec400
remove double try block
davidgamez df79395
fix permissions issues
davidgamez cba53a1
fix check availability filter
davidgamez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| """Who wrote a `gtfs_feed_availability_check` row, and which rows count as feed availability. | ||
|
|
||
| The table holds more than the daily availability check: failed nightly dataset downloads and | ||
| the download runs asked for by hand through the operations API are recorded there too, with | ||
| their request and response detail, so they can be debugged side by side. | ||
|
|
||
| Only the daily check answers "was the feed available". Every public reader of the table - | ||
| the `/v1/gtfs_feeds/{id}/availability` endpoint and the seal's `available` criterion, both in | ||
| the nightly run and in the backfill - filters through `feed_availability_checks_filter()`, so | ||
| changing which sources count is a single edit to `FEED_AVAILABILITY_SOURCES`. The operations | ||
| API is the one reader that sees every source. | ||
|
|
||
| This module is under `shared/common` so the API and every function that writes or reads the | ||
| table (`tasks_executor`, `batch_process_dataset`, `operations_api`) share one definition. | ||
| """ | ||
|
|
||
| from enum import Enum | ||
| from typing import Final, Tuple | ||
|
|
||
| from shared.database_gen.sqlacodegen_models import GtfsFeedAvailabilityCheck | ||
|
|
||
|
|
||
| class AvailabilityCheckSource(str, Enum): | ||
| """Values stored in `gtfs_feed_availability_check.source`.""" | ||
|
|
||
| # The daily lightweight check run by tasks_executor. | ||
| AVAILABILITY_CHECK = "availability_check" | ||
| # A nightly dataset download that failed, recorded by batch_process_dataset. | ||
| DATASET_DOWNLOAD = "dataset_download" | ||
| # A run on the nightly job asked for through the operations API, download-only or full, | ||
| # success included. | ||
| DATASET_DOWNLOAD_MANUAL = "dataset_download_manual" | ||
|
|
||
|
|
||
| # The sources whose rows answer "was the feed available". | ||
| FEED_AVAILABILITY_SOURCES: Final[Tuple[AvailabilityCheckSource, ...]] = (AvailabilityCheckSource.AVAILABILITY_CHECK,) | ||
|
|
||
|
|
||
| def feed_availability_checks_filter(): | ||
| """SQLAlchemy clause keeping only the checks that count as feed availability.""" | ||
| return GtfsFeedAvailabilityCheck.source.in_([source.value for source in FEED_AVAILABILITY_SOURCES]) |
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
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
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
53 changes: 53 additions & 0 deletions
53
api/tests/unittest/models/test_availability_check_source_contract.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| """Contract tests pinning `gtfs_feed_availability_check.source` values. No database connection. | ||
|
|
||
| `shared.common.availability_check_source` is the single definition of the sources the | ||
| functions write and of the ones that count as feed availability. The public availability | ||
| endpoint and the Seal of Reliability both filter through it; the operations API exposes every | ||
| source as an enum on its `source` filter. | ||
|
|
||
| Adding a source means updating, in lockstep: `AvailabilityCheckSource` and the | ||
| `availability_source` parameter in `docs/OperationsAPI.yaml` plus a stub regen. | ||
| """ | ||
|
|
||
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| import yaml | ||
|
|
||
| from shared.common.availability_check_source import ( | ||
| FEED_AVAILABILITY_SOURCES, | ||
| AvailabilityCheckSource, | ||
| feed_availability_checks_filter, | ||
| ) | ||
| from shared.database_gen.sqlacodegen_models import GtfsFeedAvailabilityCheck | ||
|
|
||
| OPERATIONS_SPEC = Path(__file__).resolve().parents[4] / "docs" / "OperationsAPI.yaml" | ||
|
|
||
|
|
||
| class TestAvailabilityCheckSourceContract(unittest.TestCase): | ||
| def test_only_the_daily_check_counts_as_feed_availability(self): | ||
| """Changing this changes the public endpoint and the seal; it should be deliberate.""" | ||
| self.assertEqual(FEED_AVAILABILITY_SOURCES, (AvailabilityCheckSource.AVAILABILITY_CHECK,)) | ||
|
|
||
| def test_filter_selects_the_feed_availability_sources(self): | ||
| clause = feed_availability_checks_filter() | ||
| compiled = clause.compile(compile_kwargs={"literal_binds": True}) | ||
| self.assertEqual(clause.left.table.name, GtfsFeedAvailabilityCheck.__tablename__) | ||
| self.assertEqual(clause.left.name, "source") | ||
| self.assertIn("'availability_check'", str(compiled)) | ||
| for source in set(AvailabilityCheckSource) - set(FEED_AVAILABILITY_SOURCES): | ||
| self.assertNotIn(f"'{source.value}'", str(compiled)) | ||
|
|
||
| def test_operations_spec_lists_every_source(self): | ||
| spec = yaml.safe_load(OPERATIONS_SPEC.read_text()) | ||
| parameter = spec["components"]["parameters"]["availability_source"] | ||
| self.assertEqual( | ||
| set(parameter["schema"]["items"]["enum"]), | ||
| {source.value for source in AvailabilityCheckSource}, | ||
| ) | ||
|
|
||
| def test_db_default_is_the_daily_check(self): | ||
| """A row written without a source must still count as an availability check.""" | ||
| column = GtfsFeedAvailabilityCheck.__table__.c.source | ||
| self.assertFalse(column.nullable) | ||
| self.assertIn(AvailabilityCheckSource.AVAILABILITY_CHECK.value, str(column.server_default.arg)) |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
[question] the
/v1/gtfs_feeds/{id}/availabilityendpoint reads from the samegtfs_feed_availability_checktable, and its query doesn't filter on the newsourcecolumn. So after this change, the public endpoint would also return the failed nightly downloads (dataset_download) and thedownload_debugdry runs and manual runs. Is that intended?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.
Great catch! fixed here
Theoretically, it can also affect the seal; in practice, when an operator is testing manually, the feed is already failing the availability check. In any case, I kept these new "downloads" actions outside the seal computation for consistency.