Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve octetDeltaCount calculation #251

Closed
wants to merge 2 commits into from

Conversation

heanlan
Copy link
Contributor

@heanlan heanlan commented Aug 26, 2021

Currently when we calculate octetDeltaCount for aggregated records, we are summing up the values from all incoming records. This approach leads to an issue that, in the case of inter-node flows, we will receive records from both source and destination node for the same flow, which makes octetDeltaCount approximate to 2*real value.

To make it more accurate, we change the update logic to:
new_octetDeltaCount = max(new_octetDeltaCountFromSource, old_octetDeltaCount), if the incoming record is from source node;
new_octetDeltaCount = max(new_octetDeltaCountFromDestination, old_octetDeltaCount), if the incoming record is from destination node

fixes: #250

@heanlan heanlan changed the title Improve octetDeltacount calculation [NotForReview]Improve octetDeltacount calculation Aug 26, 2021
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #251 (f2e5883) into main (ff181f9) will increase coverage by 0.03%.
The diff coverage is 81.81%.

❗ Current head f2e5883 differs from pull request most recent head c732167. Consider uploading reports for the commit c732167 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
+ Coverage   76.23%   76.26%   +0.03%     
==========================================
  Files          17       17              
  Lines        2365     2368       +3     
==========================================
+ Hits         1803     1806       +3     
  Misses        393      393              
  Partials      169      169              
Flag Coverage Δ
integration-tests 53.25% <54.54%> (+0.03%) ⬆️
unit-tests 74.61% <81.81%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
pkg/intermediate/aggregate.go 70.41% <81.81%> (+0.20%) ⬆️

@heanlan heanlan marked this pull request as draft August 31, 2021 19:47
@heanlan heanlan force-pushed the octet-delta-counts branch 16 times, most recently from 6906339 to c5b5011 Compare September 1, 2021 04:22
@heanlan heanlan changed the title [NotForReview]Improve octetDeltacount calculation Improve octetDeltaCount calculation Sep 1, 2021
@heanlan heanlan marked this pull request as ready for review September 1, 2021 18:51
srikartati
srikartati previously approved these changes Sep 3, 2021
Copy link
Contributor

@srikartati srikartati left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/intermediate/aggregate.go Outdated Show resolved Hide resolved
pkg/intermediate/aggregate.go Outdated Show resolved Hide resolved
Signed-off-by: heanlan <anlan9771@gmail.com>
zyiou
zyiou previously approved these changes Sep 3, 2021
Copy link
Contributor

@zyiou zyiou left a comment

Choose a reason for hiding this comment

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

LGTM. How does it work in Antrea?

@heanlan
Copy link
Contributor Author

heanlan commented Sep 3, 2021

LGTM. How does it work in Antrea?

It works well in Antrea, referring to antrea-io/antrea#2692
although I think the accuracy is approximately equally good as the previous approach.

@heanlan
Copy link
Contributor Author

heanlan commented Oct 26, 2021

follow up in #270 , closing now

@heanlan heanlan closed this Oct 26, 2021
@heanlan heanlan deleted the octet-delta-counts branch October 26, 2021 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inaccurate delta stats of aggregated records
4 participants