Skip to content

Add support for partitioning by probe_id in the window functions - #187

Merged
LDiazN merged 8 commits into
mainfrom
fix/report_id-rotation
Sep 25, 2026
Merged

LDiazN merged 8 commits into
mainfrom
fix/report_id-rotation

Conversation

@hellais

@hellais hellais commented Sep 18, 2026

Copy link
Copy Markdown
Member

Fixes: #162

@hellais
hellais requested a review from LDiazN September 18, 2026 15:12
@codecov

codecov Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.84%. Comparing base (efb0f1d) to head (3853b57).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
+ Coverage   83.65%   83.84%   +0.18%     
==========================================
  Files          89       89              
  Lines        5917     5985      +68     
==========================================
+ Hits         4950     5018      +68     
  Misses        967      967              
Flag Coverage Δ
oonidata 77.87% <ø> (ø)
oonipipeline 86.70% <ø> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread oonipipeline/tests/test_rules.py Outdated
@LDiazN

LDiazN commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Looks good in general, just a small comment. Do you think it's feasible to make a test to run this against actual data? The current test just checks the query string consistency

Demonstrate how without the change a measurement would erroneously be
marked as "IPv6 broken", while it was actually IPv6 blocking
* Fix the tests so that they don't include TLS measurements which never
  happen if TCP is failing
@hellais
hellais requested a review from LDiazN September 25, 2026 10:34
@hellais

hellais commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

I implemented some new tests and made some changes to the rules in order to distinguish IPv4 from IPv6 failures

Comment thread oonipipeline/tests/test_analysis.py
Comment thread oonipipeline/src/oonipipeline/analysis/rules.py
When that's happening we aren't triggering TCP blocking
@hellais

hellais commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

I pushed changes that:

  • Add test for broken IPv6 configuration verifying it marks tcp OK correctly
  • Add TCP failures to legacy rules

@hellais
hellais requested a review from LDiazN September 25, 2026 12:10
ok=0.0,
layer=RuleLayer.TCP,
outcome_class=OutcomeClass.DOWN,
comment="Failure against an address that is nor v4 nor v4 that mostly succeeds in the control. This should in theory never trigger",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
comment="Failure against an address that is nor v4 nor v4 that mostly succeeds in the control. This should in theory never trigger",
comment="Failure against an address that is nor v4 nor v6 that mostly succeeds in the control. This should in theory never trigger",

@LDiazN LDiazN left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 🚀

@LDiazN
LDiazN merged commit 07e13bd into main Sep 25, 2026
7 checks passed
@LDiazN
LDiazN deleted the fix/report_id-rotation branch September 25, 2026 14:12
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.

Handle the fact that each measurement might have a unique report_id

2 participants