Update Flow aggregate field httpVals #334
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
==========================================
- Coverage 73.01% 72.73% -0.29%
==========================================
Files 19 19
Lines 2831 2853 +22
==========================================
+ Hits 2067 2075 +8
- Misses 597 604 +7
- Partials 167 174 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
tushartathgur
force-pushed
the
l7flowAggregator
branch
6 times, most recently
from
a2046e4
to
daa32ea
Compare
| @@ -984,3 +993,29 @@ func isCorrelationRequired(flowType uint8, record entities.Record) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| func fillHttpVals(incomingHttpVals, existingHttpVals string) (string, error) { | |||
| incomingHttpValsJson := make(map[int32]string) | |||
There was a problem hiding this comment.
Forgive me if it is a dumb question. Transaction iD should be the same for the same event in different nodes?
There was a problem hiding this comment.
Yes transaction ID should be same
There was a problem hiding this comment.
I think that's a key point, and the commit message should probably mention it.
Given that the HTTP values from both Nodes should eventually be consistent, I wonder if there is real value in that "complex" aggregation logic, or if we should just use the "latest" record (like we do for some other fields) and simply override the aggregated value?
There was a problem hiding this comment.
I am not able to reproduce it in my local setup, but the thought behind this design is if multiple exporters send the httpvals with different or overlapping TxIDs, we should deduplicate it.
if we only take the latest record for aggregation, we may not get all the TxIDs in a aggregated output given the aggregation interval is bigger than the exporter interval.
There was a problem hiding this comment.
I think we won't be able to use the "latest" record in the current design.
The records from our Flow Exporter only contain partial httpVals as this field is reset after the connection is exported from the FE.
If we want to use the latest record in the FA, we might need to do the aggregation in the FE instead, which could end up increasing the packet size, transferred from FE to FA, by time?
There was a problem hiding this comment.
Thanks @yuntanghsu, that makes sense.
tushartathgur
force-pushed
the
l7flowAggregator
branch
2 times, most recently
from
91c21dc
to
1dbce89
Compare
tushartathgur
force-pushed
the
l7flowAggregator
branch
from
1dbce89
to
871fef5
Compare
There was a problem hiding this comment.
This is fine by me. But I'll defer to @heanlan and @yuntanghsu for approval.
pkg/intermediate/aggregate.go
Outdated
| return "", fmt.Errorf("error parsing JSON: %v", err) | ||
| } | ||
| } | ||
| if len(existingHttpValsJson) > 0 { |
There was a problem hiding this comment.
the check is not useful: if the length is 0, the the loop will simply not execute
pkg/intermediate/aggregate.go
Outdated
| existingVal := existingIeWithValue.GetStringValue() | ||
| updatedHttpVals, err := fillHttpVals(incomingVal, existingVal) | ||
| if err != nil { | ||
| klog.Errorf("httpVals could not be updated from jsons, err: %v", err) |
There was a problem hiding this comment.
| klog.Errorf("httpVals could not be updated from jsons, err: %v", err) | |
| klog.Errorf("httpVals could not be updated: %v", err) |
| @@ -984,3 +995,29 @@ func isCorrelationRequired(flowType uint8, record entities.Record) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| func fillHttpVals(incomingHttpVals, existingHttpVals string) (string, error) { | |||
There was a problem hiding this comment.
Can we add a unit test for this function to make sure we correlate two HttpVals correctly?
tushartathgur
force-pushed
the
l7flowAggregator
branch
from
871fef5
to
4947179
Compare
pkg/intermediate/aggregate_test.go
Outdated
| existingHttpVals: "", | ||
| updatedHttpVals: "{\"1\":\"{hostname:10.10.0.1,url:/public/,http_user_agent:curl/7.74.0,http_content_type:text/html,http_method:GET,protocol:HTTP/1.1,status:200,length:153}\"}", | ||
| }, { | ||
| name: "overlapping httpVals empty", |
There was a problem hiding this comment.
nit: this is the only test case name that you didn't capitalize
also what does "empty" mean in this test case?
There was a problem hiding this comment.
Allow me to change the name , "empty" just got copied from the last test case, I will change it too
if exporter sends httpval1 in first export and sends httpvals2 in second we should append both before sending further. Also the function should take care of deduplicating same TxID items. Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
tushartathgur
force-pushed
the
l7flowAggregator
branch
from
4947179
to
e23c6b5
Compare
if exporter sends httpval1 in first export and sends httpvals2 in second we should append both before sending further. Also the function should take care of deduplicating same TxID items. Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com> Co-authored-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
if exporter sends httpval1 in first export and sends httpvals2 in second we should append both before sending further. Also the function should take care of deduplicating same TxID items. Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com> Co-authored-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
if exporter sends httpval1 in first export and sends httpvals2 in second we should append both before sending further.
Also the function should take care of deduplicating same TxID items.