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: add experimental otelcol log collector #1986

Merged
merged 2 commits into from
Jan 5, 2022

Conversation

swiatekm
Copy link

@swiatekm swiatekm commented Dec 16, 2021

Description

This is an alpha feature without a stable API. It only supports container logs from docker-shim. Fluent-bit needs to be manually disabled to avoid duplicate logs.

The namespacing in the internal configuration is currently a bit messy - we have a top-level sumologic.logs key, which should probably be sumologic.metadata.logs, same with metrics. Same thing is true about the directory structure within the Helm chart. I'd like to change both of those in a separate PR after merging this one. Then this collector can live under sumologic.logs.* instead of sumologic.logs.collector.*.

The configuration for otelcol was originally prepared by @astencel-sumo, he gets credit for it actually doing the right thing.


Checklist

Remove items which don't apply to your PR.

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

@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch from b87ac45 to dc97a49 Compare December 16, 2021 15:14
@github-actions github-actions bot added the documentation documentation label Dec 16, 2021
@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch 4 times, most recently from 32209ee to 76d2776 Compare December 17, 2021 09:23
@swiatekm swiatekm marked this pull request as ready for review December 17, 2021 09:25
@swiatekm swiatekm requested a review from a team as a code owner December 17, 2021 09:25
@perk-sumo
Copy link
Contributor

perk-sumo commented Dec 17, 2021

I'd like to change both of those in a separate PR after merging this one.

Please create an issue for v3 as that will be a breaking change. Also this most probably will be changed globally for many other keys.

@swiatekm
Copy link
Author

I'd like to change both of those in a separate PR after merging this one.

Please create an issue for v3 as that will be a breaking change. Also this most probably will be changed globally for many other keys.

I meant the internal keys defined in _helpers.tpl, which aren't a breaking change and are simpler structurally (since they only cover resources explicitly defined in our chart).

deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch from 76d2776 to eb86894 Compare December 17, 2021 13:25
@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch from a517abd to 29209f0 Compare December 17, 2021 13:45
@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch 2 times, most recently from 65fbac4 to 6211128 Compare December 17, 2021 18:36
@swiatekm swiatekm requested a review from perk-sumo December 20, 2021 10:16
@swiatekm
Copy link
Author

@astencel-sumo maybe you can take a look as well?

@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch from 6211128 to c4900a4 Compare December 30, 2021 10:18
tests/helm/logs_otc/static/basic.input.yaml Outdated Show resolved Hide resolved
tests/helm/logs_otc_daemonset/static/basic.input.yaml Outdated Show resolved Hide resolved
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

LGTM except for nits from Patryk 🚀

@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch from c4900a4 to 2fb5464 Compare January 3, 2022 15:15
@swiatekm swiatekm enabled auto-merge (rebase) January 3, 2022 15:17
@swiatekm swiatekm requested a review from pmalek-sumo January 3, 2022 15:24
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
Mikolaj Swiatek and others added 2 commits January 4, 2022 14:42
This is an alpha feature without a stable API. It only supports container
logs from docker-shim.

Co-authored-by: Patryk Małek <pmalek@sumologic.com>
We're adding more pipelines that use otlp as a source, and we should
be clearer about what kind of data each pipeline processes.
@swiatekm swiatekm force-pushed the feat/otelcol-log-collector branch from a03a07e to 4cc5971 Compare January 4, 2022 13:42
@swiatekm swiatekm requested a review from pmalek-sumo January 4, 2022 13:43
@swiatekm swiatekm merged commit b053314 into main Jan 5, 2022
@swiatekm swiatekm deleted the feat/otelcol-log-collector branch January 5, 2022 10:42
@perk-sumo perk-sumo added this to the v2.4 milestone Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants