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

Postgresql discovery config - add OTel native receiver #3957

Merged

Conversation

greatestusername-splunk
Copy link
Contributor

Description: Changes the bundled test and discovery configs for postgresql to the native OTEL receiver. Disables old smartagent postgresql discovery

Link to Splunk idea: <Link to Splunk idea, see https://ideas.splunk.com>

Testing: Bundled tests updated

"--set", "splunk.discovery.receivers.postgresql.config.password=otelp",
"--set", "splunk.discovery.receivers.postgresql.config.tls::insecure=true",
//Disabling postgresql.backends metric that doesn't dependably show up in metrics during testing
"--set", "splunk.discovery.receivers.postgresql.config.metrics::postgresql.backends::enabled=false",
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 was strange. During testing I'd have a 50/50 shot of this postgresql.backends metric showing up or not when pulling metrics for comparison.
Tried longer startup times and different methods of generating connections to try to get this metric to consistently show up but couldn't find any real rhyme or reason.

Open to ideas if it is imperative we have this enabled for testing.

@greatestusername-splunk greatestusername-splunk marked this pull request as ready for review November 22, 2023 14:16
@greatestusername-splunk
Copy link
Contributor Author

Should be all set to go unless there are more requested changes.

@atoulme atoulme changed the title Postgresql discovery config - swap to OTel native receiver Postgresql discovery config - add OTel native receiver Dec 8, 2023
@rmfitzpatrick
Copy link
Contributor

It'd be helpful to add an entry to https://github.com/signalfx/splunk-otel-collector/blob/main/cmd/otelcol/config/collector/config.d.linux/properties.discovery.yaml.example (I hope to automate this w/ the bundlegen process in the future)

…ostgresql.discovery.yaml.tmpl

Co-authored-by: Ryan Fitzpatrick <[email protected]>
@greatestusername-splunk
Copy link
Contributor Author

It'd be helpful to add an entry to https://github.com/signalfx/splunk-otel-collector/blob/main/cmd/otelcol/config/collector/config.d.linux/properties.discovery.yaml.example (I hope to automate this w/ the bundlegen process in the future)

Do you want me to remove the smartagent/postgresql entry or just set it enabled: false in the example? I'd guess remove that smart agent one fully and replace it with the native postgresql one. But wanted to verify.

@rmfitzpatrick
Copy link
Contributor

Do you want me to remove the smartagent/postgresql entry or just set it enabled: false in the example? I'd guess remove that smart agent one fully and replace it with the native postgresql one. But wanted to verify.

I'd just set the enabled to false instead of removing so that the file can be a quick place to look for the bundled components. I plan on using the tooling updates in #4066 as a base to automatically generate the example property content w/ that in mind, but if you'd prefer they not be included instead it'd be helpful to know why.

@greatestusername-splunk
Copy link
Contributor Author

I think this is just a flakey test issue?

go: CreateFile C:\Users\RUNNER~1\AppData\Local\Temp\go-build1388815158\b2783\settings.test.exe: Access is denied.

https://github.com/signalfx/splunk-otel-collector/actions/runs/7213743861/job/19654281668?pr=3957

@rmfitzpatrick
Copy link
Contributor

@greatestusername-splunk sorry, I've shifted things from under you. Do you mind ~rebasing and adding the receiver to the new https://github.com/signalfx/splunk-otel-collector/blob/main/internal/confmapprovider/discovery/bundle/components.go#L33 before running make bundle.d once more?

@@ -16,4 +16,6 @@ splunk.discovery:
smartagent/collectd/nginx:
enabled: true
smartagent/postgresql:
enabled: false
postgresql:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, alpha ordering plz

@greatestusername-splunk
Copy link
Contributor Author

@greatestusername-splunk sorry, I've shifted things from under you. Do you mind ~rebasing and adding the receiver to the new https://github.com/signalfx/splunk-otel-collector/blob/main/internal/confmapprovider/discovery/bundle/components.go#L33 before running make bundle.d once more?

no prob! Should be all set

- instrumentation_scope:
attributes: {}
name: otelcol/postgresqlreceiver
version: <ANY>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version: <ANY>
version: <VERSION_FROM_BUILD>

nit, though there's a handful of other instances as well

@rmfitzpatrick
Copy link
Contributor

Mind adding a breaking change changelog item and update the discovery readme to include the component? https://github.com/signalfx/splunk-otel-collector/tree/main/internal/confmapprovider/discovery#discovery-mode

I'll look into automating a table along w/ the example properties automation work.

Copy link
Contributor

@rmfitzpatrick rmfitzpatrick left a comment

Choose a reason for hiding this comment

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

lgtm w/ final requests.

@greatestusername-splunk
Copy link
Contributor Author

Mind adding a breaking change changelog item and update the discovery readme to include the component? https://github.com/signalfx/splunk-otel-collector/tree/main/internal/confmapprovider/discovery#discovery-mode

I'll look into automating a table along w/ the example properties automation work.

In and ready. 😄

@jvoravong jvoravong merged commit 740f745 into signalfx:main Dec 18, 2023
131 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants