Avoid false positives in rtrmon when VRPs are removed #72
Conversation
However, time does change so we need to account for the new thresholds and grace period.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Time still passes so VRPs may be dropped due to the expiration of the grace period.
| firstSeen := tCurrentUpdate | ||
| currentEntry, ok := currentVrps[key] | ||
| if ok { | ||
| if ok && currentEntry.Visible { |
There was a problem hiding this comment.
Let's add a comment? I think this updates lastSeen to the current time, when an item was in grace period, but is seen in the real source again?
cmd/rtrmon/rtrmon.go
Outdated
| continue | ||
| } | ||
|
|
||
| updated := *vrp |
There was a problem hiding this comment.
Is updated identical to VRP or not? Does it get copied onto the heap here?
(I think it is fine to mutate these objects as long as we have the lock, but do also see why you may not want this)
| } else if isDefinitelyNotVisisble(vrp, thresholdTimestamp) { | ||
| // VRP is definitely not visible, count if other side is definitely visible | ||
| if isDefinitelyVisible(other, thresholdTimestamp) { | ||
| count++ |
There was a problem hiding this comment.
I assumed we we not counting the symmetric difference, but the one-directional difference.
You can get the symmetric difference by summing over lhs/rhs?
There was a problem hiding this comment.
We are counting the one-directional difference, since the input to this function is from a one-directional difference operation. The other side is only used to check for the current visibility based on the VRPs timeline and the threshold.
| onlyInA := make(VRPMap) | ||
| for key, vrpA := range a { | ||
| if vrpA.Visible { | ||
| if vrpB, ok := b[key]; !ok || !vrpB.Visible { |
There was a problem hiding this comment.
We now account for visibility here, think that is the important change of this PR. Things that were no longer visible still popped up in the difference.
There was a problem hiding this comment.
Yes, the maps contain VRPs that are no longer visible and we need to ignore these when calculating the current difference.
|
It feels like we already capture |
|
Thank you for your time and contributions @erikrozendaal and @ties |
3 participants
Track visibility of VRPs on both sides and only consider them different when the visibility does not match based on the visibility thresholds.
Also update the results even when the sources have not been modified, since the thresholds may have changed in the meantime as time passes.