-
Notifications
You must be signed in to change notification settings - Fork 14
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-1407 console deduper merge mode #435
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #435 +/- ##
==========================================
- Coverage 58.61% 58.34% -0.28%
==========================================
Files 167 167
Lines 8312 8385 +73
Branches 1061 1074 +13
==========================================
+ Hits 4872 4892 +20
- Misses 3132 3182 +50
- Partials 308 311 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/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=51c5a2d make set-plugin-image |
I sometimes get I'm adapting the plugin to manage both in between |
config/sample-config.yaml
Outdated
@@ -812,3 +818,6 @@ frontend: | |||
alertNamespaces: | |||
- netobserv | |||
sampling: 50 | |||
deduper: | |||
mark: false | |||
merge: true |
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.
isn't better to stay with the current default till we change it ?
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.
Sure !
@@ -122,6 +128,10 @@ func ReadConfigFile(version, date, filename string) (*Config, error) { | |||
Filters: []Filter{}, | |||
QuickFilters: []QuickFilter{}, | |||
Features: []string{}, | |||
Deduper: Deduper{ |
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.
can u pls add a note here so when the agent default change we need to remember and change this as well ? OR it will be much better to have only one source of truth can u read it from the same cm instead of hard coding it here ?
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've added a TODO for now. We could load by default the sample yaml but that would force us to embed it in the code and parse two yaml, one after the other.
The plugin accept as arguments a file path that is configurable. These defaults are overriden below (line 141)
return metricType == constants.MetricTypeBytes || | ||
metricType == constants.MetricTypePackets | ||
func shouldMergeReporters(metricType constants.MetricType, deduper loki.Deduper) bool { | ||
return !deduper.Merge && deduper.Mark && (metricType == constants.MetricTypeBytes || |
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 don't think it make any sense to have mark true
and merge true
at the sametime
while we don't enforce this yet but this is something we should consider
IMO valid configs
Mark | Merge
T | F
F | T
since we always need to know about dup so both T or both F is invalid, maybe we can just single config knob at the agent side ?
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.
Both merge
and mark
could be false 🙃
But I agree both true
doesn't make sense on eBPF side. I just wanted to manage every cases here since we have 2 flags for now.
I suggest to keep as is and cleanup the code for enablement in https://issues.redhat.com/browse/NETOBSERV-1459
pkg/loki/filter.go
Outdated
@@ -141,7 +142,7 @@ func (f *lineFilter) asLabelFilters() []labelFilter { | |||
valueType: v.valueType, | |||
value: v.value, | |||
} | |||
if v.valueType == typeRegex || v.valueType == typeRegexContains { | |||
if v.valueType == typeRegex || v.valueType == typeRegexContains || v.valueType == typeRegexArrayContains { |
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.
maybe worth small helper to check for all regex flavor ?
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.
sure !
pkg/loki/filter.go
Outdated
@@ -141,7 +142,7 @@ func (f *lineFilter) asLabelFilters() []labelFilter { | |||
valueType: v.valueType, | |||
value: v.value, | |||
} | |||
if v.valueType == typeRegex || v.valueType == typeRegexContains { | |||
if v.valueType == typeRegex || v.valueType == typeRegexContains || v.valueType == typeRegexArrayContains { | |||
if f.not { | |||
lf.matcher = labelNoMatches | |||
} else { |
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.
not related to this commit but I think u can avoid if else here just init lf.matcher and check if !f.not
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.
sounds good to me
default: | ||
isArray = false | ||
} | ||
} |
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 don't understand this its always an array if merge is on ?
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.
Yes !
Currently, when deduper merged is enabled, we have both single and plural fields as:
FlowDirection, Interface, FlowDirections, Interfaces
I'm forcing a `lineMatch`` on plural fields here to avoid any query running on single fields.
In the future, I expect fully removing FlowDirection and Interface fields.
if ( | ||
flow.fields.Interfaces && | ||
flow.fields.FlowDirections && | ||
flow.fields.Interfaces.length === flow.fields.FlowDirections.length |
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.
why u need this check its not possible the two arrays of different len ?
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.
It's not supposed to happen indeed, but just in case, I'm checking the length to avoid crashing in the loop following this test since I'm accessing values by their index.
? t('Ingress') | ||
: value === FlowDirection.Egress | ||
? t('Egress') | ||
: value === FlowDirection.Inner |
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.
can we ever get Inner
flow direction its either ingress or egress right ?
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.
Inner
is something calculated from reinterpret direction in FLP.
It's not yet adapted to plural fields
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.
sounds good to me Thanks @jpinsonneau |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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
mark
&merge
configsTo setup deduper merge, simply add the following config in ebpf debug (or advanced) configuration:
Dependencies
netobserv/network-observability-operator#501
netobserv/network-observability-operator#529
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.