Skip to content

Decoupling the change detection and alerting logic#9381

Closed
MohamedBilelBesbes wants to merge 0 commit intomozilla:masterfrom
MohamedBilelBesbes:BotInterface
Closed

Decoupling the change detection and alerting logic#9381
MohamedBilelBesbes wants to merge 0 commit intomozilla:masterfrom
MohamedBilelBesbes:BotInterface

Conversation

@MohamedBilelBesbes
Copy link
Copy Markdown
Contributor

This PR is dedicated to separate the logics of the change point detection and the alerting.

  • Separated the logic of change point detection from the logic of alert creation: run_detection handles pure detection with no database side effects, and create_alerts_from_results is the sole function responsible for persisting alerts, making it possible to query the detection pipeline without triggering any alerting (meaning that Sherlock could use it).
  • Made run_detection configurable to operate in two modes: a voting mode (voting_strategy="equal" or "priority") that runs the full ensemble and applies a consensus strategy, and a single-method mode (voting_strategy=None) that returns change points flagged directly by one named CPD method without involving the voting system.
  • Moved the responsibility of selecting which methods participate in the voting system from build_cpd_methods into generate_new_test_alerts_in_series via the voting_methods param.
  • Updated the existing deduplication guard unit test to match the new decoupling logic, where build_revision_data and run_detection are now called explicitly and separately instead of through the previously coupled vote function.
  • Added a new unit test to verify the single-method mode of run_detection, using Mann Whitney U as the detector and asserting that results come exclusively from that method's change_detected flags with no voting involvement.

Copy link
Copy Markdown
Collaborator

@gmierz gmierz left a comment

Choose a reason for hiding this comment

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

Good changes! Some things to fix below. I may have some more comments incoming.

Comment thread treeherder/perf/alerts.py Outdated
Comment thread treeherder/perf/alerts.py Outdated
Comment thread treeherder/perf/alerts.py Outdated
Comment thread treeherder/perf/alerts.py Outdated
Comment thread treeherder/perf/alerts.py Outdated
@MohamedBilelBesbes
Copy link
Copy Markdown
Contributor Author

@gmierz I accidently closed the PR upo trying to sync this branch to current master, I did address the comments writtewn in this PR, I will create a new PR with the changes.

@MohamedBilelBesbes
Copy link
Copy Markdown
Contributor Author

Link to new PR here

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