diff --git a/main.py b/main.py index 0da8372..c94b6b8 100755 --- a/main.py +++ b/main.py @@ -22,10 +22,12 @@ import jwt import requests +import sentry_sdk from google.api_core import exceptions as api_exceptions from google.api_core.client_options import ClientOptions from google.auth.credentials import AnonymousCredentials from google.cloud import bigquery +from sentry_sdk.integrations.logging import LoggingIntegration BUG_RE = re.compile(r"\b(?:bug|b=)\s*#?(\d+)\b", re.I) @@ -362,12 +364,12 @@ def get_installation_access_token( expected_status=201, ) + # Never include resp.text in these errors: the body may carry the + # token, and error messages are reported to Sentry. try: data = resp.json() except requests.exceptions.JSONDecodeError as e: - raise RuntimeError( - f"Failed to parse access token response: {e}: {resp.text}" - ) + raise RuntimeError(f"Failed to parse access token response: {e}") try: access_token = AccessToken( token=data["token"], @@ -375,7 +377,8 @@ def get_installation_access_token( ) except KeyError as e: raise RuntimeError( - f"Unexpected access token response structure, missing key {e}: {resp.text}" + f"Unexpected access token response structure, missing key {e}; " + f"keys present: {sorted(data)}" ) except ValueError as e: raise RuntimeError( @@ -398,6 +401,31 @@ def setup_logging() -> None: ) +def setup_sentry() -> bool: + """ + Report errors to Sentry when SENTRY_DSN is set; no-op otherwise. + + Log records at INFO and above are attached as breadcrumbs only; events are + sent explicitly with sentry_sdk.capture_exception/capture_message so each + failure is reported exactly once (several paths log an error and then raise). + SENTRY_ENVIRONMENT and SENTRY_RELEASE are read from the environment by the SDK. + + Returns: + True if Sentry was initialized, False if SENTRY_DSN is unset. + """ + dsn = os.environ.get("SENTRY_DSN") + if not dsn: + return False + sentry_sdk.init( + dsn=dsn, + integrations=[LoggingIntegration(level=logging.INFO, event_level=None)], + # Tracebacks pass through frames holding the GitHub private key, app JWT + # and installation tokens; never ship frame locals to Sentry. + include_local_variables=False, + ) + return True + + def _parse_github_timestamp(value: str | None) -> datetime | None: """ Parse a GitHub ISO-8601 timestamp (e.g. ``2026-06-17T14:53:58Z``) into an @@ -1711,6 +1739,26 @@ def _flush_pending() -> None: return processed +def _process_repo_in_sentry_scope(repo: str, *args, **kwargs) -> int: + """ + Run process_repo for *repo* inside its own Sentry isolation scope. + + Runs in the worker thread so the retry/backoff warnings logged while + processing the repo are attached as breadcrumbs to the failure event (Sentry + scopes are per-thread, and pool threads are reused across repos). The event + is tagged with the repo for filtering. Exceptions are logged, reported, then + re-raised so the caller can record the repo as failed. + """ + with sentry_sdk.isolation_scope() as scope: + scope.set_tag("repo", repo) + try: + return process_repo(repo, *args, **kwargs) + except Exception as exc: + logger.exception(f"Failed to process repo {repo}: {exc}") + sentry_sdk.capture_exception(exc) + raise + + def main() -> int: """ Main ETL process with chunked processing. @@ -1722,14 +1770,23 @@ def main() -> int: 4. Repeat until no more data """ setup_logging() + sentry_enabled = setup_sentry() try: - return _main() + return _main(sentry_enabled) except RuntimeError as e: - logger.error(str(e)) + logger.exception(str(e)) + sentry_sdk.capture_exception(e) return 1 + except SystemExit as e: + # Missing configuration: report it, then still fail the run. + sentry_sdk.capture_message(str(e), level="fatal") + raise + finally: + # The process exits right after this; send queued events first. + sentry_sdk.flush() -def _main() -> int: +def _main(sentry_enabled: bool = False) -> int: logger.info("Starting GitHub ETL process with chunked processing") github_app_id = os.environ.get("GITHUB_APP_ID") or None @@ -1810,7 +1867,7 @@ def _main() -> int: with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: future_to_repo = { executor.submit( - process_repo, + _process_repo_in_sentry_scope, repo, github_app_id, github_private_key, @@ -1829,13 +1886,12 @@ def _main() -> int: repo = future_to_repo[future] try: processed = future.result() - except Exception as exc: + except Exception: # Catch broadly so one repo's failure (a TooManyRetriesError, a # RuntimeError, or a bare Exception from load_data) is recorded as a # failed repo rather than propagating out of the executor and - # discarding the results of other in-flight repos. logger.exception - # records the worker thread's traceback for debugging in CI/prod. - logger.exception(f"Failed to process repo {repo}: {exc}") + # discarding the results of other in-flight repos. The worker has + # already logged/reported it (see _process_repo_in_sentry_scope). failed_repos.append(repo) continue total_processed += processed @@ -1844,10 +1900,13 @@ def _main() -> int: ) if failed_repos: - logger.error( + logger.warning( f"ETL completed with failures. Failed repos: {', '.join(failed_repos)}" ) - return 1 + # With Sentry configured, per-repo failures were reported there, so exit 0 + # to avoid marking the cron job as failed. Without it, the exit code is the + # only signal, so fail the run. + return 0 if sentry_enabled else 1 logger.info( f"GitHub ETL process completed successfully. Total PRs processed: {total_processed}" diff --git a/pyproject.toml b/pyproject.toml index e729a88..8d444bc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,6 +21,7 @@ dependencies = [ "requests>=2.25.0", "google-cloud-bigquery==3.25.0", "PyJWT[crypto]>=2.0.0", + "sentry-sdk>=2.0.0", ] [project.optional-dependencies] diff --git a/requirements.txt b/requirements.txt index 0196810..a65d4e0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,7 +7,9 @@ certifi==2026.1.4 \ --hash=sha256:9943707519e4add1115f44c2bc244f782c0249876bf51b6599fee1ffbedd685c \ --hash=sha256:ac726dd470482006e014ad384921ed6438c457018f4b3d204aea4281258b2120 - # via requests + # via + # requests + # sentry-sdk cffi==2.0.0 \ --hash=sha256:00bdf7acc5f795150faa6957054fbbca2439db2f775ce831222b66f192f03beb \ --hash=sha256:07b271772c100085dd28b74fa0cd81c8fb1a3ba18b21e03d7c27f3436a10606b \ @@ -458,6 +460,10 @@ rsa==4.9.1 \ --hash=sha256:68635866661c6836b8d39430f97a996acbd61bfa49406748ea243539fe239762 \ --hash=sha256:e7bdbfdb5497da4c07dfd35530e1a902659db6ff241e39d9953cad06ebd0ae75 # via google-auth +sentry-sdk==2.71.0 \ + --hash=sha256:7beb27a22f06396f3a05510c3da6ea10e1c79e02e859993c27ce4ff074e296d6 \ + --hash=sha256:9ed36b4c206048aa67b5ddba0a6e28d59fc55cf10d60f5f7aea2c3917184bf4a + # via github-etl (pyproject.toml) six==1.17.0 \ --hash=sha256:4721f391ed90541fddacab5acf947aa0d3dc7d27b2e1e8eda2be8970586c3274 \ --hash=sha256:ff70335d468e7eb6ec65b95b99d3a2836546063f63acc5171de367e834932a81 @@ -469,4 +475,6 @@ typing-extensions==4.15.0 \ urllib3==2.6.3 \ --hash=sha256:1b62b6884944a57dbe321509ab94fd4d3b307075e0c2eae991ac71ee15ad38ed \ --hash=sha256:bf272323e553dfb2e87d9bfd225ca7b0f467b919d7bbd355436d3fd37cb0acd4 - # via requests + # via + # requests + # sentry-sdk diff --git a/tests/test_main.py b/tests/test_main.py index 20fb2b3..feff4f3 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,8 +1,11 @@ +import logging import os import threading +import uuid from unittest.mock import MagicMock, Mock, patch import pytest +import sentry_sdk import main @@ -540,7 +543,7 @@ def extract_side_effect(*args, **kwargs): ): result = main.main() - assert result == 1 # partial failure + assert result == 1 # no SENTRY_DSN, so the exit code must signal the failure assert mock_extract.call_count == 2 # both repos were attempted mock_load.assert_called_once() # only the successful repo loaded data @@ -562,7 +565,7 @@ def test_bare_exception_on_one_repo_is_isolated( """A bare Exception (e.g. from load_data) on one repo must not abort the others. The executor catches broadly, so the failing repo is recorded in failed_repos - (overall exit code 1) while the healthy repo still completes its load. + (exit code 1 without SENTRY_DSN) while the healthy repo still completes its load. """ # Fresh iterator per repo (a shared return_value iterator would be exhausted # by whichever repo consumes it first). @@ -602,6 +605,137 @@ def transform_side_effect(chunk, repo): assert mock_load.call_count == 2 # both repos reached the load step +@patch("main.sentry_sdk") +@patch("main.setup_logging") +@patch("main.bigquery.Client") +@patch("requests.Session") +@patch("main.extract_pull_requests") +def test_repo_failure_is_tagged_for_sentry( + mock_extract, + mock_session_class, + mock_bq_client, + mock_setup_logging, + mock_sentry, +): + """A failed repo is reported under a Sentry scope tagged with the repo name, + and the run exits 0 because Sentry is configured.""" + mock_extract.side_effect = main.TooManyRetriesError("GitHub API error 502") + scope = mock_sentry.isolation_scope.return_value.__enter__.return_value + + with patch.dict( + os.environ, + { + "SENTRY_DSN": "https://key@o0.ingest.sentry.io/0", + "GITHUB_REPOS": "mozilla/firefox", + "BIGQUERY_PROJECT": "test", + "BIGQUERY_DATASET": "test", + }, + clear=True, + ): + result = main.main() + + assert result == 0 + scope.set_tag.assert_called_once_with("repo", "mozilla/firefox") + # Reported exactly once, even though the failure is also logged. + mock_sentry.capture_exception.assert_called_once() + mock_sentry.flush.assert_called_once() + + +@patch("main.sentry_sdk") +@patch("main.setup_logging") +def test_missing_config_is_reported_and_still_fails(mock_setup_logging, mock_sentry): + """Missing required env vars are sent to Sentry and still exit non-zero.""" + with patch.dict(os.environ, {}, clear=True): + with pytest.raises(SystemExit): + main.main() + + mock_sentry.capture_message.assert_called_once() + assert "BIGQUERY_PROJECT" in mock_sentry.capture_message.call_args.args[0] + mock_sentry.flush.assert_called_once() + + +@patch("main.sentry_sdk") +@patch("main.setup_logging") +@patch("main._main", side_effect=RuntimeError("token exchange failed")) +def test_runtime_error_still_fails(mock_main, mock_setup_logging, mock_sentry): + """A top-level RuntimeError still returns a non-zero exit code.""" + assert main.main() == 1 + mock_sentry.capture_exception.assert_called_once() + mock_sentry.flush.assert_called_once() + + +class TestSetupSentry: + """Tests for setup_sentry initialization.""" + + @patch("main.sentry_sdk.init") + def test_noop_without_dsn(self, mock_init): + with patch.dict(os.environ, {}, clear=True): + assert main.setup_sentry() is False + mock_init.assert_not_called() + + @patch("main.sentry_sdk.init") + def test_initializes_with_dsn(self, mock_init): + dsn = "https://key@o0.ingest.sentry.io/0" + with patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True): + assert main.setup_sentry() is True + mock_init.assert_called_once() + assert mock_init.call_args.kwargs["dsn"] == dsn + + def test_does_not_send_local_variables(self): + """Frame locals hold the private key, JWT and tokens. Never send them.""" + # Generated at runtime so it can't leak via source context lines, + # only via frame locals. + secret = uuid.uuid4().hex + events = [] + + class CapturingTransport(sentry_sdk.transport.Transport): + def capture_envelope(self, envelope): + events.append(envelope.get_event()) + + real_init = sentry_sdk.init + + def init_with_capturing_transport(*args, **kwargs): + return real_init(*args, transport=CapturingTransport(), **kwargs) + + def fail_with_secret_in_scope(): + private_key = secret # noqa: F841 + raise RuntimeError("token exchange failed") + + dsn = "https://key@o0.ingest.sentry.io/0" + try: + with ( + patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True), + patch( + "main.sentry_sdk.init", side_effect=init_with_capturing_transport + ), + ): + assert main.setup_sentry() is True + try: + fail_with_secret_in_scope() + except RuntimeError: + sentry_sdk.capture_exception() + finally: + sentry_sdk.get_client().close() + sentry_sdk.get_global_scope().set_client(None) + + assert len(events) == 1 + frames = events[0]["exception"]["values"][-1]["stacktrace"]["frames"] + assert any(f["function"] == "fail_with_secret_in_scope" for f in frames) + assert all("vars" not in f for f in frames) + assert secret not in repr(events[0]) + + @patch("main.LoggingIntegration") + @patch("main.sentry_sdk.init") + def test_logs_are_breadcrumbs_only(self, mock_init, mock_logging_integration): + """Log records must not become events; failures are captured explicitly.""" + dsn = "https://key@o0.ingest.sentry.io/0" + with patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True): + main.setup_sentry() + mock_logging_integration.assert_called_once_with( + level=logging.INFO, event_level=None + ) + + class TestResolveMaxWorkers: """Tests for _resolve_max_workers worker-count resolution.""" diff --git a/tests/test_token_store.py b/tests/test_token_store.py index 24c59fe..4c043ea 100644 --- a/tests/test_token_store.py +++ b/tests/test_token_store.py @@ -6,10 +6,15 @@ when X-RateLimit-Remaining is 0. - TokenStore: caches valid tokens, expires tokens within the 60s skew window, and hands out a stable per-installation lock. + - get_installation_access_token: error messages for malformed token responses + never include the response body (it may carry the token). """ from datetime import datetime, timedelta, timezone -from unittest.mock import Mock +from unittest.mock import MagicMock, Mock, patch + +import pytest +import requests import main from main import AccessToken, TokenStore, _is_rate_limited @@ -82,3 +87,30 @@ def test_token_store_lock_is_stable_per_installation(): def test_module_level_token_store_exists(): assert isinstance(main.token_store, TokenStore) + + +@pytest.mark.parametrize( + "json_side_effect", + [ + # Token present but expires_at missing. + [{"token": "ghs_secret"}], + # Body is not valid JSON. + requests.exceptions.JSONDecodeError("bad", '{"token": "ghs_secret"', 0), + ], +) +def test_access_token_errors_do_not_leak_response_body(json_side_effect): + """Error messages reach Sentry, so they must never contain the token.""" + resp = Mock() + resp.json.side_effect = json_side_effect + resp.text = '{"token": "ghs_secret"' + + with ( + patch("main._build_session", return_value=MagicMock()), + patch("main.github_request", return_value=resp), + patch("main.token_store", TokenStore()), + patch.dict(main.repo_installation_cache, {"mozilla/firefox": 1}), + ): + with pytest.raises(RuntimeError) as excinfo: + main.get_installation_access_token("jwt", "mozilla/firefox", "https://api") + + assert "ghs_secret" not in str(excinfo.value)