m-lab / etl

M-Lab ingestion pipeline
Apache License 2.0
22 stars 7 forks source link

Fix bug with snapshot thinning. #1104

Closed NotSpecial closed 1 year ago

NotSpecial commented 1 year ago

In the current version, the snapshot thinning does not work properly because the n%10 != 0 check, intended to always add the final snapshot, suffers from an off-by-one error.

Concretely, the main loop in the function appends snapshots at index 0, 10, 20, 30, ...; i.e. the first element and every tenth after. Second, the final check tests whether the list of snapshots is a multiple of 10, and if not adds the final element. And this is where the problem lies: the loop does not take every tenth element, it takes the first one and then every tenth after, i.e. the 11th, 21st, 31st, etc -- it is essentially shifted by 1, and thus the n%10 check is incorrect. This leads to the following behavior: If the list has a length that is a multiple of 10, the final element will be excluded. If it has a length of a multiple of 10 plus 1, the final element will be duplicated. Here's an illustration:

IN:  []
OUT: []
---
IN:  [0]
OUT: [0, 0]
---
IN:  [0, 1]
OUT: [0, 1]
---
IN:  [0, 1, 2]
OUT: [0, 2]
---
IN:  [0, 1, 2, 3]
OUT: [0, 3]
---
IN:  [0, 1, 2, 3, 4]
OUT: [0, 4]
---
IN:  [0, 1, 2, 3, 4, 5]
OUT: [0, 5]
---
IN:  [0, 1, 2, 3, 4, 5, 6]
OUT: [0, 6]
---
IN:  [0, 1, 2, 3, 4, 5, 6, 7]
OUT: [0, 7]
---
IN:  [0, 1, 2, 3, 4, 5, 6, 7, 8]
OUT: [0, 8]
---
IN:  [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
OUT: [0]
---
IN:  [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
OUT: [0, 10, 10]
---
IN:  [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]
OUT: [0, 10, 11]

In this PR, I simplify the code somewhat to avoid the index check: The main loop only considers elements up to, but excluding the last one (i < n-1). Afterwards, the last element is always added.


This change is Reviewable

NotSpecial commented 1 year ago

TODO: A unittest would be nice. I don't have much experience with unittess in go, but I can give it a shot next week.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 7416


Changes Missing Coverage Covered Lines Changed/Added Lines %
parser/tcpinfo.go 4 6 66.67%
<!-- Total: 4 6 66.67% -->
Files with Coverage Reduction New Missed Lines %
active/active.go 2 90.63%
storage/storage.go 154 0%
<!-- Total: 156 -->
Totals Coverage Status
Change from base Build 7409: -3.9%
Covered Lines: 3009
Relevant Lines: 4756

💛 - Coveralls
NotSpecial commented 1 year ago

Mh, just by looking at the test file, I cannot tell where the expected number of 1588 snapshots comes from. Probably needs some extra investigation.

What I want to say: As the counting has been wrong, I am not sure whether 1588 is the correct number to expect.

stephen-soltesz commented 1 year ago

This change was incorporated into https://github.com/m-lab/etl/pull/1105