-
Notifications
You must be signed in to change notification settings - Fork 23
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
NETOBSERV-1208 & NETOBSERV-1233 Aggregators skip missing fields #470
Conversation
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=e10d50b make set-flp-image |
Codecov Report
@@ Coverage Diff @@
## main #470 +/- ##
==========================================
- Coverage 66.12% 66.06% -0.07%
==========================================
Files 94 94
Lines 6867 6877 +10
==========================================
+ Hits 4541 4543 +2
- Misses 2066 2072 +6
- Partials 260 262 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
if agg.metrics != nil { | ||
agg.metrics.aggregatorErrors.WithLabelValues("MissingFieldError", agg.inputField).Inc() | ||
// error only if explicitly specified as FLP skip empty fields by default to reduce storage size | ||
if agg.reportMissing { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume netobserv app always expect this bool to be false
? when can this bool be true
I am wondering if we even need to bool and we can omit the error always ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this could be usefull even for netobserv
.
FlowDirection
, IfDirection
, TimeFlowStartMs
, fields for example.Bytes
, Packets
@@ -179,11 +185,13 @@ func (agg *aMax) update(conn connection, flowLog config.GenericMap, d direction, | |||
} | |||
|
|||
func (cp *aFirst) update(conn connection, flowLog config.GenericMap, d direction, isNew bool) { | |||
if isNew { | |||
if isNew && flowLog[cp.inputField] != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume if we don't allow by default empty fields then we won't need this check as well as the one in the update function ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal here is to avoid reporting empty fields for first
and last
operations (just below this one).
Without it, these will create field with nil:
c.aggFields[fieldName] = newValue |
I'm not sure to get your point. Can you please clarify ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing it seems better to work directly in the toGenericMap
function
74611d2
That avoid to manage per aggregate cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msherif1234 any reason not skipping zeros for Bytes
and Packets
fields ?
de53e15
For consistency it would be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no @jpinsonneau it makes sense to skip empty fields
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=5f298ac make set-flp-image |
@@ -100,7 +101,9 @@ func (c *connType) getNextHeartbeatTime() time.Time { | |||
func (c *connType) toGenericMap() config.GenericMap { | |||
gm := config.GenericMap{} | |||
for k, v := range c.aggFields { | |||
gm[k] = v | |||
if v != nil && (reflect.TypeOf(v).Kind() != reflect.Float64 || v.(float64) != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be && i.e
if v != nil && reflect.TypeOf(v).Kind() == reflect.Float64 && v.(float64) != 0 {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aggFields
can be other types than float64
here
Example: string
for PktDropLatestState
or PktDropLatestDropCause
on last
aggregate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
merging this to unlock operator PR |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpinsonneau The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Following #451 we removed unused fields to the genericMap by default.
Aggregators on connection tracking needs to be updated to:
This PR introduce a new
ReportMissing
field optional to allow error reporting on aggregations.Please let me know if you want to have a different default behavior or a more flexible solution.