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

Remove monitor_agent events #122

Merged
merged 1 commit into from
May 4, 2021
Merged

Remove monitor_agent events #122

merged 1 commit into from
May 4, 2021

Conversation

dmitryax
Copy link
Contributor

@dmitryax dmitryax commented May 4, 2021

Internal metrics already exposed via Prometheus that should be used for monitoring instead of emitting monitor_agent events.

@jrcamp
Copy link
Contributor

jrcamp commented May 4, 2021

Did you verify that prometheus isn't depending on monitor_agent being configured? It's in their sample config but I don't know if it's required or just a coincidence: https://github.com/fluent/fluent-plugin-prometheus/blob/master/misc/fluentd_sample.conf

If it's required I think we can remove the tag config option to make it stop emitting events: https://docs.fluentd.org/input/monitor_agent#tag

@@ -1,6 +1,6 @@
apiVersion: v2
name: splunk-otel-collector
version: 0.24.12
version: 0.24.13
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 5 PRs open at this time. What is the reason to include a version bump along with the change itself as opposed to a separate release commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the helm chart, we don't follow the rule to have a release commit so far. I don't see a reason to do that at the moment. Just to introduce a change that can be utilized right away. Git tags can be used to track history of releases instead of release commits.

@jrcamp what's your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'm open to having release commits if everyone agrees

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 bumping the version in the same PR is fine. If there are multiple changes ready to go out it would be nice to group them into the same release just to avoid superfluous releases. I doubt it's bothering anybody though.

CHANGELOG.md Outdated Show resolved Hide resolved
@dmitryax
Copy link
Contributor Author

dmitryax commented May 4, 2021

If it's required I think we can remove the tag config option to make it stop emitting events: https://docs.fluentd.org/input/monitor_agent#tag

@jrcamp I tested it. it's not required

@dmitryax dmitryax force-pushed the remove-monit-events branch from 5d2608e to 3c26385 Compare May 4, 2021 16:04
Internal metrics already exposed via Prometheus that should be used for monitoring instead of emitting monitor_agent events.
@dmitryax dmitryax force-pushed the remove-monit-events branch from 3c26385 to 24f9b34 Compare May 4, 2021 16:08
@dmitryax dmitryax merged commit 79d6701 into main May 4, 2021
@dmitryax dmitryax deleted the remove-monit-events branch May 4, 2021 21:01
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