-
Notifications
You must be signed in to change notification settings - Fork 183
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
feat: refactor OT log collector configuration #2436
Conversation
## merge-multiline-logs merges incoming log records into multiline logs. | ||
## Input Body (JSON): { "log": "2001-02-03 04:05:06 first line\n", "stream": "stdout" } | ||
## Input Body (JSON): { "log": " second line\n", "stream": "stdout" } | ||
## Input Body (JSON): { "log": " third line\n", "stream": "stdout" } | ||
## Output Body (JSON): { "log": "2001-02-03 04:05:06 first line\n second line\n third line\n", "stream": "stdout" } | ||
{{- if .Values.sumologic.logs.multiline.enabled }} | ||
- id: merge-multiline-logs | ||
type: recombine | ||
output: extract-metadata-from-filepath | ||
source_identifier: attributes["log.file.path"] | ||
combine_field: body.log | ||
combine_with: "" | ||
is_first_entry: body.log matches {{ .Values.sumologic.logs.multiline.start_regex | quote }} | ||
{{- end }} |
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 would reconsider supporting multiple mutltiline detections in this PR as this is common use case for customers. I can also create Issue if we prefer to add it in the future
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 that should be added in a separate PR. Sounds like a complicated use case.
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.
Overall this is fine. Are we going to document whose changes in this PR or separate one?
I believe we should start with the documentation (this PR doesn't add or modify any). Writing the documentation should help us make sure the configuration properties that we define in |
4d208fb
to
ee596d4
Compare
2c585a4
to
137c419
Compare
93f2238
to
314d872
Compare
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, please get approval from someone else as well :)
5dbf317
to
ee0e812
Compare
{{/* | ||
Check if otelcol logs collector is enabled. | ||
It's enabled if both logs in general and the collector specifically are enabled. | ||
If both the collector and Fluent-Bit are enabled, we error. |
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 really want to explicitly forbid the combination of both OT logs collector and Fluent Bit?
I can imagine a user might e.g. want to collect container logs with OT and systemd logs with FB (or the other way around) for some reason and I'm not convinced that hardcoding the lack of this possibility is a good idea.
Don't get me wrong, I can see big value in making it impossible for the user to make silly mistakes, but this specific example seems too restrictive for me.
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 that customers more likely forget to disable one of the collector during migration than will try to use both of them. Especially when we deprecate fluent-bit
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 the value of catching accidental misconfigurations is greater than the value of allowing users to have both simultaneously. Now that OT supports systemd logs, I don't think there's any reason to run them side-by-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.
I think we should allow for both collectors running at the same time but behind a feature flag, something like allowBothCollectors: 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.
I added it as allowSideBySide
, along with a documentation section.
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.
aa489cf
to
209ab42
Compare
Co-authored-by: Dominik Rosiek <58699848+sumo-drosiek@users.noreply.github.com> Co-authored-by: Andrzej Stencel <astencel@sumologic.com>
209ab42
to
e22e06c
Compare
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 as long as we agree that we want to forbid the simultaneous usage of Fluent Bit and OT logs collector. I was against forbidding it, but if everyone is fine with it, I'm good.
After all, this can be changed later if we change our minds (or the customers change our minds).
ee963d0
to
b7f0f80
Compare
b7f0f80
to
14dd2a6
Compare
Description
Change how log collection using OT is configured. The idea here is for the purely functional configuration to live under the
sumologic.logs
key and contain no application-specific configuration, and for the K8s specific and application specific (like raw OT configuration) configuration to be separate - currently I've left it underotellogs
, but I'm open to alternatives.TODO:
Checklist
Testing performed