Skip to content

refactor: remove sentry logging and integration #349#351

Merged
peterdudfield merged 5 commits intoopenclimatefix:mainfrom
danishdynamic:cleanup/remove-sentry
Mar 16, 2026
Merged

refactor: remove sentry logging and integration #349#351
peterdudfield merged 5 commits intoopenclimatefix:mainfrom
danishdynamic:cleanup/remove-sentry

Conversation

@danishdynamic
Copy link
Copy Markdown
Contributor

Pull Request

Description

This PR removes the Sentry SDK and all associated logging logic as part of the transition to Apitally (Issue #349).

Key Changes:

  • Code Cleanup: Removed write_sentry calls from forecast.py and deleted the utils/sentry_logging.py helper.

  • Dependency Management: Removed sentry-sdk from pyproject.toml to reduce the package footprint.

  • Logic Preservation: Verified that latitude/longitude rounding (previously used for Sentry masking) was not required for core forecasting and removed it from the telemetry flow.

Fixes #349

How Has This Been Tested?

  1. Syntax & Import Check: Ran python -m compileall . to ensure no broken imports or syntax errors remained after deleting the Sentry utility.

  2. Dependency Verification: Confirmed that sentry-sdk is no longer in the project's dependency list.

  3. Manual Code Review: Verified that the rounding of latitude/longitude in the old write_sentry function was only for data masking in Sentry and does not affect the actual forecast data processing.

I performed a structural and syntax-based verification to ensure the removal of the Sentry module did not leave any "zombie" imports or broken logic chains.

  1. Reproduction Steps:
  • Syntax Compilation: Ran python -m compileall . to verify that all files (especially forecast.py) compile without ImportError or ModuleNotFoundError related to the deleted sentry_logging.py.

  • Import Trace: Verified via python -c "import quartz_solar_forecast.forecast" that the main entry point no longer attempts to initialize Sentry.

  • Manual Audit: Checked tests/unit/test_forecast.py and tests/unit/test_generate_forecast.py for any leftover write_sentry mocks or calls.

  1. Test Configuration:
  • Local OS: Windows 11

  • Python Version: 3.14.0 (Note: Local full test suite execution was limited by dependency compatibility with Python 3.14, but syntax integrity was verified. I am relying on the CI/CD pipeline for the final Python 3.11/3.12 integration test run).

  • Environment: Virtualenv (.venv)

  • No (Changes were strictly to logging/telemetry, not the forecasting algorithm itself).

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@danishdynamic
Copy link
Copy Markdown
Contributor Author

I have successfully removed Sentry and migrated the telemetry to Apitally. I noticed the CI/CD was failing due to a stale date in test_run_forecast_historical (exceeding the 3-month XGB limit). I'm currently syncing a fix to update that test window to 30 days so the checks can pass. Apologies for the extra commits while I resolve a local Git sync issue.

@peterdudfield
Copy link
Copy Markdown
Contributor

Thanks @danishdynamic , this looks great. You happy for this to be merged?

@danishdynamic
Copy link
Copy Markdown
Contributor Author

Thanks @peterdudfield ! I'm happy for it to be merged.

One CI check is still failing because the historical test originally used a timestamp 200 days in the past, but the XGB model now rejects dates older than 3 months. I updated the test to use a 60 day timestamp and removed the explicit timestamp for the XGB call so it defaults to the current time.

I'm pushing another commit to make sure GitHub Actions runs against the latest version of the test. Once that finishes, CI should go green.

@danishdynamic
Copy link
Copy Markdown
Contributor Author

Thanks again @peterdudfield for the feedback!

I've pushed commits updating the historical test to use a 60 day window and removed the timestamp from the XGB call so it defaults to the current time. The changes are visible in the PR diff.

However, the CI job still seems to be running the earlier version of the test (with the 200-day timestamp). Since the main Sentry removal change looks good, please feel free to re-run the workflow or merge if you're satisfied with the code.

Happy to make any further adjustments if needed!

@peterdudfield peterdudfield merged commit 0d3042d into openclimatefix:main Mar 16, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

remove: sentry

2 participants