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 OT event collection #2379

Merged
merged 1 commit into from
Jul 4, 2022
Merged

feat: add experimental OT event collection #2379

merged 1 commit into from
Jul 4, 2022

Conversation

swiatekm
Copy link

@swiatekm swiatekm commented Jun 15, 2022

Description

This adds event collection using OT. As we don't have an events subsection in the sumologic section, I can't implement a simple switch like we have for metadata, and instead need to add a new section and depend on the user not to enable both at once.

One thing of note is the approach to configuring OT. Instead of simply embedding the configuration in values.yaml, we instead treat it as a normal template, and then allow overriding it via a configuration field whose function will be similar to the current approach, but which is intended as a last resort escape hatch rather than the default way of configuring OT.

TODO:

  • documentation
  • comments for the more interesting features

Checklist
  • Changelog updated
Testing performed
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@github-actions github-actions bot added the documentation documentation label Jun 20, 2022
@swiatekm swiatekm added this to the v2.11 milestone Jun 20, 2022
@swiatekm swiatekm force-pushed the feat/ot-events branch 3 times, most recently from 06de4ee to d1a172d Compare June 21, 2022 12:15
@swiatekm swiatekm marked this pull request as ready for review June 21, 2022 12:21
@swiatekm swiatekm requested a review from a team as a code owner June 21, 2022 12:21
@swiatekm
Copy link
Author

Note: this currently includes #2388, I'll rebase once that's merged to make the config table changes clearer.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains invalid labels. Please remove all of the following labels: ['do-not-merge/hold']

@swiatekm
Copy link
Author

This depends on SumoLogic/sumologic-otel-collector#620, and therefore probably on the 0.54.0 OT distro release.

@sumo-drosiek
Copy link
Contributor

I would insist to test all possible values for statefulset (including sumologic key and so on)

@sumo-drosiek
Copy link
Contributor

We can consider provider approach, to avoid both otelevents and fluentd being run at the same time

@swiatekm
Copy link
Author

We can consider provider approach, to avoid both otelevents and fluentd being run at the same time

That's a breaking change though, as events don't have a section under sumologic. We can switch to that in 3.0.

@sumo-drosiek
Copy link
Contributor

it is not until fluentd will be default choice

@swiatekm
Copy link
Author

it is not until fluentd will be default choice

It is. If someone disables events by setting fluentd.events.enabled to false, and I add sumologic.events.enabled defaulting to true, it's either enabled, in which case it's a breaking change, or not, in which case we have a situation very similar to right now.

@sumo-drosiek
Copy link
Contributor

I was thinking rather about flag sumologic.events.provider

@swiatekm
Copy link
Author

swiatekm commented Jun 22, 2022

I was thinking rather about flag sumologic.events.provider

Does that actually help? We run into problems if someone enables both FluentD and OT events. In that case, sumologic.events.provider could be a tiebreaker, but it's confusing that you need to set this and separately enable the thing you want.

@sumo-drosiek
Copy link
Contributor

we could enable otc events by default and control it by sumologic.events.provider only, especially if we plan to have this flag in the future. If not, we can skip it

@swiatekm swiatekm merged commit c712362 into main Jul 4, 2022
@swiatekm swiatekm deleted the feat/ot-events branch July 4, 2022 13:49
andrzej-stencel added a commit that referenced this pull request Jul 6, 2022
This builds on event collection with OpenTelemetry Collector introduced in #2379, but changes how event collection is configured. It does so by removing the property `otelevents.enabled` and instead adding `sumologic.events.enabled` and `sumologic.events.provider`.

Also adding some more documentation around this and fixing some typos or misleading docs.

With this change, every property does exactly one thing:

- `sumologic.events.enabled` enables or disables events collection
- `sumologic.events.provider` switches between Fluentd and Otelcol

We also still support the (now deprecated) `fluentd.events.enabled` property to disable events collection with Fluentd.
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

2 participants