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 event collection configuration #2444

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Conversation

swiatekm
Copy link

@swiatekm swiatekm commented Jul 21, 2022

Description

Move event configuration to the sumologic.events section, containing common configuration for OT and FluentD. This should also include switching to otelcol as the default provider, but I thought this would be easier to review without that change.

With this change, provider-agnostic configuration for events includes:

  • sourceName and sourceCategory
  • persistence

I'm wondering if I shouldn't also expose something to more easily modify the OT pipeline - for example by letting the user add processors to the end of it. WDYT?


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 Jul 21, 2022
@swiatekm swiatekm added this to the v3.0 milestone Jul 22, 2022
@swiatekm swiatekm force-pushed the feat/ot-events-config branch 3 times, most recently from 00876ed to fcd9d10 Compare July 22, 2022 10:28
@swiatekm swiatekm marked this pull request as ready for review July 22, 2022 10:34
@swiatekm swiatekm requested a review from a team as a code owner July 22, 2022 10:34
@sumo-drosiek
Copy link
Contributor

I think we should mark it as breaking-change as there are changes in fluentd buffer path

@sumo-drosiek
Copy link
Contributor

I'm wondering if I shouldn't also expose something to more easily modify the OT pipeline - for example by letting the user add processors to the end of it. WDYT?

We can add optional metadata enrichment and filtering as well, but generally wouldn't do anything unless we have explicit request/usecase for it

@swiatekm
Copy link
Author

I think we should mark it as breaking-change as there are changes in fluentd buffer path

It's a breaking change for many reasons, but I've also already marked it as such accoring to conventional commits - what else did you have in mind?

@sumo-drosiek
Copy link
Contributor

gh label

Comment on lines +82 to +84
size: 10Gi
path: /var/lib/storage/events
accessMode: ReadWrite
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this go to otelevents configuration?
It sounds like more advanced and technical configuration

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 nice, but not sure if persistence should go to the events or otelevents configuration

@swiatekm
Copy link
Author

Config migration PR: SumoLogic/sumologic-kubernetes-tools#305

@swiatekm
Copy link
Author

Overall this is nice, but not sure if persistence should go to the events or otelevents configuration

As per offline discussion, I left persistence in the common config, but split off the K8s-specific bits.

@sumo-drosiek
Copy link
Contributor

Please make linters happy :)

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.

one nit

@swiatekm swiatekm enabled auto-merge (rebase) November 16, 2022 10:26
@swiatekm swiatekm merged commit 0d6fb49 into main Nov 16, 2022
@swiatekm swiatekm deleted the feat/ot-events-config branch November 16, 2022 10:35
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.

3 participants