-
-
Notifications
You must be signed in to change notification settings - Fork 518
Introduce sentry-good_job integration #2751
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
base: master
Are you sure you want to change the base?
Conversation
…urations - Added .gitignore to exclude unnecessary files and directories. - Created .rspec and .rubocop.yml for testing and code style configurations. - Introduced CHANGELOG.md for tracking changes and features. - Set up Gemfile and gemspec for dependency management. - Implemented core integration logic in lib/sentry-good_job.rb and related modules. - Added example application and setup scripts for demonstration. - Included comprehensive test suite for validation of functionality. This commit establishes the foundation for the sentry-good_job gem, enabling error tracking and monitoring for ActiveJob workers using Good Job.
- Added .rspec_status to .gitignore to prevent tracking of RSpec status files. - Deleted the existing .rspec_status file to clean up unnecessary artifacts from the repository.
- Updated README.md to clarify the manual setup process for job monitoring in job classes and initializers. - Removed automatic setup of job monitoring for ApplicationJob from the integration logic in lib/sentry-good_job.rb. - Adjusted tests to reflect the change, ensuring that job monitoring is not automatically set up and can be manually configured instead. - Maintained backward compatibility by keeping the main entry point in lib/sentry/good_job.rb.
- Simplified retry logic in the error handler by using a default retry attempt value instead of relying on job class configuration. - Updated the `retryable?` method to determine retryability based on job execution count rather than the presence of retry configuration. - Adjusted related tests to reflect changes in job class definitions and execution counts for better clarity and accuracy.
…tion - Improved cron expression parsing to handle various timezone formats, including multi-level IANA timezones and UTC offsets. - Updated error handling logic to better determine when to report job failures based on retry counts, introducing a new method to check if retries have been exhausted. - Added comprehensive tests for new timezone handling and retry exhaustion logic to ensure robustness and accuracy.
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.
This is a big PR to review, but briefly:
- I'm not sure of the necessity of JobMonitor and ErrorHandler. Those seem like Active Job monitors, not GoodJob ones.
- I also think CronMonitor is arguably Active Job related, if the intention is to alert when a set of jobs fails to perform on a regular schedule (Cron Monitoring). GoodJob is the implementation, but that doesn't seem inherent to GoodJob.
- On naming, I've intentionally avoided using "dead" in GoodJob because I think it's an inappropriate technical metaphor. If it can be avoided here, that would be nice.
|
|
||
| # Get the job class | ||
| job_class = begin | ||
| job_class_name.constantize |
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.
It would be nice if the job classes weren't constantized. If there's any chance this is initialized during development, this would negatively impact app boot.
| cron_config.each do |job_name, job_config| | ||
| setup_monitoring_for_job(job_name, job_config) | ||
| end |
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.
That's not correct. Cron config is { unique_key: **config }. The unique_key is not a job class. It doesn't look like it's used as such, but it might be clearer.
|
@bensheldon Hey! Thank you for reading the code.
True, I was actually wondering how does sentry-sidekiq resolve this in situation when it is used as ActiveJob provider. Let's use sentry-rails implementation instead.
ActiveJob does not have any relation to GoodJob scheduler (if it will be, then it should be under sentry-rails). CronMonitoring class is required for Sentry Cron monitors feature. That's one of much waited thing.
This one will be fixed, thank you for clarification, I fully agree. |
- Changed `report_only_dead_jobs` to `report_only_discarded_jobs` for clarity, indicating that it now reports errors for jobs that have been discarded (failed and cannot be retried). - Updated related documentation in CHANGELOG.md and README.md to reflect the new terminology. - Adjusted configuration and error handling logic to ensure consistency with the updated option name. - Modified tests to validate the new configuration behavior.
|
@bensheldon if we keep it simple, then only cron feature support is required, but sentry-rails has very simple implementation on context enrichment, it takes only basic things from ActiveJob parameters. Might make sense separately improve sentry-rails implementation. |
…iveJob functionality - Removed deprecated configuration options related to job error reporting and tracing, now handled by sentry-rails. - Introduced GoodJob-specific context and tag enhancements for better integration with ActiveJob. - Updated documentation in CHANGELOG.md and README.md to reflect changes in configuration and functionality. - Implemented cron monitoring setup for scheduled jobs, improving the integration's capabilities. - Adjusted tests to validate the new behavior and ensure compatibility with the updated integration logic.
- Introduced a mechanism to prevent duplicate setup of cron monitoring by tracking setup state with a class variable. - Added a method to reset the setup state for testing purposes. - Updated the integration logic to ensure proper initialization checks for Rails applications. - Adjusted tests to utilize the new reset method, ensuring accurate monitoring setup during tests.
- Specify error types for JSON serialization and parsing exceptions to enhance clarity in error handling. - Update comments to reflect the specific errors being rescued, ensuring better understanding of the error management strategy.
- Updated the cron monitoring setup to log the names of successfully added scheduled jobs, improving clarity in monitoring output. - Modified the `setup_monitoring_for_job` method to return the job name upon successful setup, facilitating better tracking of job configurations. - Adjusted tests to verify the updated logging behavior, ensuring accurate reporting of scheduled jobs.
…ntegration - Updated the `setup_monitoring_for_job` method to return `nil` instead of `nil` for better clarity in error handling. - Removed redundant checks for job class ancestors to streamline the monitoring setup process. - Enhanced logging for job class resolution failures, improving the overall robustness of the cron monitoring integration.
- Removed redundant `**options` argument from `Sentry.start_transaction` calls in multiple integrations, including Delayed Job, Active Job, Rack, and Sidekiq. - Streamlined transaction initialization for improved clarity and consistency across the Sentry integrations.
- Improved transaction initialization logic to avoid creating a new transaction for Sentry::SendEventJob, even when trace propagation headers are present. - Enhanced retry count calculation to ensure it only accesses the `executions` attribute if it is defined and an integer. - Added a new test to verify that a SendEventJob does not create a new transaction when trace propagation headers are used.
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.
Thanks for the PR! I've left some initial feedback 🙂
| # Centralized logging utility for Sentry Good Job integration | ||
| module Sentry | ||
| module GoodJob | ||
| module Logger |
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.
There's no need for this - we have a standard SDK logger available as Sentry.configuration.sdk_logger
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.
I agree, I forgot to switch from custom logger to Sentry. :)
|
|
||
| require "spec_helper" | ||
|
|
||
| RSpec.describe Sentry::GoodJob::ActiveJobExtensions do |
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.
I'd recommend removing this spec entirely and making sure that more high-level tests that don't need mocking cover the same functionality. Otherwise this spec is very brittle and can also easily give false-positive results.
|
|
||
| require "spec_helper" | ||
|
|
||
| RSpec.describe Sentry::GoodJob::ContextHelpers do |
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.
Same recommendation as above :)
- Removed deprecated logging configuration options and centralized logging under the standard Sentry SDK logger. - Updated documentation in CHANGELOG.md and README.md to reflect the removal of `logging_enabled` and `logger` options. - Enhanced logging statements throughout the integration to utilize the new logging approach, improving consistency and clarity in log output. - Adjusted tests to verify the updated logging behavior and ensure compatibility with the refactored integration.
…a handling - Added checks to ensure the proper inclusion of the sentry-rails ActiveJobExtensions module. - Refactored span data handling to utilize a new private method for setting GoodJob-specific span data. - Updated context enhancement methods to streamline the integration with GoodJob attributes. - Removed outdated test files and added new integration tests to validate the functionality of GoodJob extensions. - Ensured compatibility with the latest changes in the Sentry and GoodJob integrations.
- Added dependencies for `sentry-rails` and `good_job` in the installation instructions. - Enhanced configuration section with optional logging setup for debugging. - Updated usage examples to reflect the new `sentry_monitor_check_ins` method for cron job monitoring. - Clarified documentation regarding the integration's automatic setup and logging capabilities.
- Added tests to verify that user context is preserved after job execution, both for successful and failed jobs, in both GoodJob and ActiveJob integrations. - Updated the Sentry ActiveJob integration to store and restore the current user context during job execution, ensuring consistency across job processing. - Improved integration tests to validate the correct behavior of user context handling in various scenarios.
…ions - Deleted tests that verify user context preservation after job execution in both GoodJob and ActiveJob integrations, as the functionality is already covered by existing tests. - Cleaned up the Sentry ActiveJob integration by removing unnecessary user context handling code, streamlining the job execution process.
Overview
This PR introduces an integration for GoodJob that provides error capture with enriched context, performance monitoring, and cron monitoring for applications using GoodJob as their ActiveJob backend.
Features
Error Capture & Performance Monitoring
Cron Monitoring
sentry_cron_monitormethodConfiguration Options
include_job_arguments: Control whether job arguments are included in error context (default: false)report_after_job_retries: Only report errors after all retry attempts (default: false)report_only_dead_jobs: Only report errors for jobs that cannot be retried (default: false)auto_setup_cron_monitoring: Automatically set up cron monitoring (default: true)logging_enabled: Enable detailed logging for debugging (default: false)Architecture
JobMonitorclass handling error capture, performance monitoring, and cron setupProduction Testing
This integration is actively tested in production environments and handles:
Configuration Example
Compatibility
Key Differences from Other Integrations