Skip to content

Implementing & using a detector class template#9216

Open
MohamedBilelBesbes wants to merge 5 commits intomozilla:masterfrom
MohamedBilelBesbes:CPDMethods
Open

Implementing & using a detector class template#9216
MohamedBilelBesbes wants to merge 5 commits intomozilla:masterfrom
MohamedBilelBesbes:CPDMethods

Conversation

@MohamedBilelBesbes
Copy link

The change implemented is making an abstract class BaseDetector which enables generic alert detection using different methods. Implementating an alert detection methods using a concrete class inheriting from BaseDetector:

  • Enables defining a custom alert detection logic per method using calculate_alpha function.
  • Enables attrributing custom parameters for the given method (min_back_window, max_back_window, fore_window, alert_threshold, etc.) without overriding them if they are signature-specific.
  • Enables the control over including a magnitude-of-change check or not.

The concrete class StudentTMagDetector inheriting from BaseDetector is the implementation of the current process for alert detection.

@gmierz gmierz self-requested a review February 11, 2026 16:59

if signature.alert_notify_emails:
send_alert_emails(signature.alert_notify_emails.split(), alert, summary)
create_alerting(signature, student_t_mag_method, analyzed_series)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of modifying this generate_new_alert_in_series method, can you make a new generate_new_test_alerts_in_series method and add your new code to there? I'd rather not touch the code that's currently being used in production for now.

It would be better to call it here like so (after the current call):

generate_new_alerts_in_series(signature)

try:
    generate_new_test_alerts_in_series(...)
except Exception:
    logger.warning(...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also used PerformanceAlertSummaryTesting and PerformanceAlertTesting in the new commit instead of the currently used PerformanceAlertSummary and PerformanceAlert classes.

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.

2 participants