Skip to content

Disable default search metrics #39068

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

Merged

Conversation

shalper2
Copy link
Contributor

Description

Removing default metrics (i.e. setting all metrics to enabled: false). This is primarily to prevent unwanted ad-hoc searches from running on Splunk Enterprise deployments which could have unforeseen side effects.

Documentation

The README.md was updated to reflect these changes.

@shalper2 shalper2 marked this pull request as ready for review March 31, 2025 21:23
@shalper2 shalper2 requested a review from a team as a code owner March 31, 2025 21:23
@shalper2 shalper2 force-pushed the disable-default-search-metrics branch from ee0256a to 761eebf Compare April 1, 2025 14:34
@@ -20,6 +20,8 @@ jobs.

## Configuration

By default the Splunk Enterprise receiver is not configured to gather any metrics (i.e. there are no metrics enabled by default).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe bold this? This is going to get asked about because reading is hard :(

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 made it bold!

@shalper2 shalper2 force-pushed the disable-default-search-metrics branch from 761eebf to ced2562 Compare April 1, 2025 17:59
Copy link
Contributor

@greatestusername greatestusername left a comment

Choose a reason for hiding this comment

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

Approved, thanks for getting this in Sam! I know that one Prometheus test we can't control pass/fail.
@michael-burt for awareness also

Copy link
Contributor

@dehaansa dehaansa left a comment

Choose a reason for hiding this comment

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

One question (that can be a follow up PR) - the scrapeSearchArtifacts scrape function appears to run its search whether or not the metrics are enabled, and only record them if enabled. I'm not familiar enough with splunk enterprise to know if that would be a concern.

component: splunkenterprisereceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "disabled default metrics to prevent unexpected Splunk enterprise behavior"
Copy link
Contributor

@dehaansa dehaansa Apr 2, 2025

Choose a reason for hiding this comment

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

Suggested change
note: "disabled default metrics to prevent unexpected Splunk enterprise behavior"
note: "disabled all metrics other than splunk.health by default to ensure all searches run on Splunk enterprise are opt-in"

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 the existing language is a little confusing, I've offered an alternative but feel free to iterate or keep it the same if you feel strongly.

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 is a good callout -- was trying to be brief but maybe was a little too brief

@michael-burt
Copy link
Contributor

michael-burt commented Apr 2, 2025

One question (that can be a follow up PR) - the scrapeSearchArtifacts scrape function appears to run its search whether or not the metrics are enabled, and only record them if enabled. I'm not familiar enough with splunk enterprise to know if that would be a concern.

This is a good call out @dehaansa. We should fix this, however it is important to note that scrapeSearchArtifacts is not firing off searches, it is calling the REST API.

@dehaansa dehaansa added ready to merge Code review completed; ready to merge by maintainers and removed waiting-for-code-owners labels Apr 2, 2025
@dehaansa
Copy link
Contributor

dehaansa commented Apr 2, 2025

Oops - went and confirmed the codeowners list and realized that the gang's all here 👍 . Marked ready for merge.

@shalper2
Copy link
Contributor Author

shalper2 commented Apr 2, 2025

One question (that can be a follow up PR) - the scrapeSearchArtifacts scrape function appears to run its search whether or not the metrics are enabled, and only record them if enabled. I'm not familiar enough with splunk enterprise to know if that would be a concern.

oh shoot great catch Sam. Let me fix this before merging. Like @michael-burt pointed out it shouldn't have too much of an impact but I think that if something is disabled a user should expect it to behave like its not there.

@shalper2 shalper2 force-pushed the disable-default-search-metrics branch from ced2562 to ddfae9d Compare April 2, 2025 19:22
@songy23 songy23 merged commit ee50355 into open-telemetry:main Apr 2, 2025
170 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 2, 2025
dmathieu pushed a commit to dmathieu/opentelemetry-collector-contrib that referenced this pull request Apr 8, 2025
#### Description
Removing default metrics (i.e. setting all metrics to `enabled: false`).
This is primarily to prevent unwanted ad-hoc searches from running on
Splunk Enterprise deployments which could have unforeseen side effects.

#### Documentation
The `README.md` was updated to reflect these changes.
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
#### Description
Removing default metrics (i.e. setting all metrics to `enabled: false`).
This is primarily to prevent unwanted ad-hoc searches from running on
Splunk Enterprise deployments which could have unforeseen side effects.

#### Documentation
The `README.md` was updated to reflect these changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/splunkenterprise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants