NETOBSERV-1597: skip RecordKeyMissing error #660
Conversation
jotak
changed the title
|
@jpinsonneau: This pull request references NETOBSERV-1597 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| m.errorsCounter.WithLabelValues("RecordKeyMissing", info.Name, info.ValueKey).Inc() | ||
| return nil | ||
| // No value means 0 to keep storage lightweight | ||
| return 0 |
There was a problem hiding this comment.
we should still return nil, I think: for counters, returning 0 or nil will have the same effect (ie. adding nothing to the counter) but for histograms, return 0 would actually corrupt the stats computation
There was a problem hiding this comment.
It depends of the histograms I guess... Do you have some concrete examples in mind ?
For data consistency we should probably add filters before doing such assumptions. For example, DNS metrics must have DNSId, DNSLatencyMs fields etc. That's what we do on plugin query side.
There was a problem hiding this comment.
All the histograms that we use are latency stuff, so I don't see a case where we would want to say: no latency == 0ms. If a flow is missing RTT, or Dns latency, then we should just ignore it for histogram stats. Expecting an explicit filter would add extra processing and could easily be error-prone IMO. I'd return your question: do you have an example in mind where we need to say nil == 0 for an histogram ?
There was a problem hiding this comment.
bytes, packets and their respective drops fields must be considered as 0 when keys are not present.
That can happen when a flow is fully sent / fully dropped
There was a problem hiding this comment.
yes but that's fine these are counters not histograms, adding 0 to the counter is equivalent to ignoring it
jotak
added
the
ok-to-test
|
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=db58132 make set-flp-image |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #660 +/- ##
==========================================
- Coverage 66.52% 66.49% -0.04%
==========================================
Files 104 104
Lines 6659 6658 -1
==========================================
- Hits 4430 4427 -3
- Misses 1915 1916 +1
- Partials 314 315 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/label qe-approved |
|
@jpinsonneau: This pull request references NETOBSERV-1597 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.17.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
thanks @memodi |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak 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 |
Description
Skip all
RecordKeyMissingand consider these as0since we removed empty fields from eBPF agent.https://github.com/netobserv/netobserv-ebpf-agent/blob/release-1.5/pkg/decode/decode_protobuf.go#L61-L67
If this needs to be configurable for specific cases please let me know.
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.