Skip to content
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

Merged
merged 3 commits into from Aug 30, 2022

Conversation

swiatekm-sumo
Copy link
Contributor

@swiatekm-sumo swiatekm-sumo commented Jul 15, 2022

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 under otellogs, but I'm open to alternatives.

TODO:

  • Add template tests for the new options
  • setting for tailling additional files
  • documentation

Checklist
  • Changelog updated
Testing performed
  • Confirm events, logs, and metrics are coming in

Comment on lines 118 to 135
## 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 }}
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a 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?

@andrzej-stencel
Copy link
Contributor

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 values.yaml cover the scenarios we want to cover and that the names/structure make sense.

@swiatekm-sumo swiatekm-sumo changed the title feat: refactor OT logging configuration feat: refactor OT log collector configuration Aug 24, 2022
@swiatekm-sumo swiatekm-sumo force-pushed the feat/otellogs-configuration branch 2 times, most recently from 2c585a4 to 137c419 Compare August 25, 2022 11:45
@swiatekm-sumo swiatekm-sumo marked this pull request as ready for review August 25, 2022 11:46
@swiatekm-sumo swiatekm-sumo requested a review from a team as a code owner August 25, 2022 11:46
@github-actions github-actions bot added the documentation documentation label Aug 25, 2022
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a 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 :)

{{/*
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.
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swiatekm-sumo and others added 2 commits August 29, 2022 15:50
Co-authored-by: Dominik Rosiek <58699848+sumo-drosiek@users.noreply.github.com>
Co-authored-by: Andrzej Stencel <astencel@sumologic.com>
Copy link
Contributor

@andrzej-stencel andrzej-stencel left a 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).

@swiatekm-sumo swiatekm-sumo enabled auto-merge (rebase) August 30, 2022 07:34
@swiatekm-sumo swiatekm-sumo merged commit 4b206ad into main Aug 30, 2022
@swiatekm-sumo swiatekm-sumo deleted the feat/otellogs-configuration branch August 30, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants