-
Notifications
You must be signed in to change notification settings - Fork 251
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
Add options in FluentOperator helm chart to add more systemdFilter in Fluentbit Systemd Input and a condition on systemdFilter to enable/disable #785
Conversation
Signed-off-by: karan k <[email protected]> fix fix
07d5231
to
677ce61
Compare
@@ -15,10 +15,15 @@ spec: | |||
db: /fluent-bit/tail/systemd.db | |||
dbSync: Normal | |||
systemdFilter: | |||
{{- if .Values.fluentbit.input.systemd.systemdFilter.enable }} |
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 think we can remove .Values.fluentbit.input.systemd.systemdFilter.enable
switch because if it's disabled, then all systemd logs will be collected which is too much
I agree that users can use .Values.fluentbit.input.systemd.systemdFilter.filters
to collect more systemd logs
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.
@benjaminhuo That enable switch is added to make the systemdFilter
optional, so that Customer can get the logs from all the services. is there any particular reason of not allowing the customer to fetch all the Systemd logs? Anyway, Systemd is gonna read all the logs , systemdFilter
comes top of it for filter purpose. I don't think, there will less processing if we remove all the filters. Correct me I am wrong. As of now, we are forcing customer to get logs only from the specified services. But some customer don't want to use any filter, they just want to get the logs from all the services. .Values.fluentbit.input.systemd.systemdFilter.enable
is for that use case. Also it is true by default, so behaviour would be same as it is now.
let me know if that makes sense, otherwise I'll remove this flag.
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.
@karan56625 If the requirement is to fetch all systemd logs by disabling systemdFilter
, then it's ok to add that switch. My concern is that it might be too many logs collected in this case when users disable this and we want users to know what exactly logs they want to collect by adding the default systemdFilter
and allow them to add more filters
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.
Actually that's the requirement, to fetch all systemd logs by disabling systemdFilter. As of now, I don't see any option in Helm chart to achieve that. Also for customer, it is difficult to make list of expected services from which they gonna fetch the logs. If they have the list of expected service , they can just update the systemdFilter, otherwise if they want, they can fetch logs from all service. I don't see any issue there. That's an additional stuff for customer. By default, we are not fetching all the logs. but just adding option to achieve that.
Let me know, how should we conclude this. If we have disagreement on .Values.fluentbit.input.systemd.systemdFilter.enable
, I can remove this.
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.
Understood, if this is the requirements, we can add this switch
@@ -147,6 +147,9 @@ fluentbit: | |||
readFromHead: false | |||
systemd: | |||
enable: true | |||
systemdFilter: | |||
enable: 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.
Do we need an enable flag 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.
@adiforluls as I've commented, we do not need a enable flag here:
…ilter_based_on_condition Add options in FluentOperator helm chart to add more systemdFilter in Fluentbit Systemd Input and a condition on systemdFilter to enable/disable
…ilter_based_on_condition (#7) Add options in FluentOperator helm chart to add more systemdFilter in Fluentbit Systemd Input and a condition on systemdFilter to enable/disable
What this PR does / why we need it:
Add condition on systemdFilter in fluentbit and add options to add more systemdFilter in fluentOperator helm chart
Which issue(s) this PR fixes:
Fixes #786
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: