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

Add back and expose source category for events source #377

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

maimaisie
Copy link
Contributor

Description

Since the events data doesn't go through metadata enrichment plugin currently, the source category for events source is just the default http after we removed static source category for all sources in #349.

This PR adds it back just for events source and expose it in values.yaml file. Since after Sam's breaking change, all events configuration fields go under events I am adding it here in case this goes out in a minor release before the major one to avoid one more rename that customer needs to do when we do make the major release.

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in
  • Ran helm template to verify overwriting the field

@@ -85,6 +85,10 @@ sumologic:
# If enabled, collect K8s events
eventCollectionEnabled: true

events:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we move eventCollectionEnabled under this, maybe as:

events:
  collectionEnabled: true
  sourceCategory: ""

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be changed after #349

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused, #349 is already merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry wrong one, it is #330, specifically this change

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks for confirming

@@ -85,6 +85,10 @@ sumologic:
# If enabled, collect K8s events
eventCollectionEnabled: true

events:
# Source category for the Events source. Default: "{clusterName}/events"
sourceCategory: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use what we had below to set the default sourceCategory: "%{namespace}/%{pod_name}"

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 am using the source category we had for events before for consistency. Also what would pod name be in this case? For logs it's the name of the pod that logs are coming from I assume, but events are not only about pods. We can't use the pod name of the events pod itself either, since we don't know the pod name at the time of the source creation in TF

@maimaisie maimaisie merged commit 5ce1a72 into master Jan 30, 2020
@maimaisie maimaisie deleted the maisie-events-source-category branch January 30, 2020 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants