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

Network interface configuration #3421

Merged
merged 1 commit into from
Aug 10, 2023
Merged

Network interface configuration #3421

merged 1 commit into from
Aug 10, 2023

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jul 28, 2023

Add support for configuring the network interface on which the collector receivers will listen

@atoulme atoulme requested review from a team as code owners July 28, 2023 07:03
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.

You've added a new required env var without any enforcement on service startup (breaking). Please add to https://github.com/signalfx/splunk-otel-collector/blob/main/internal/settings/settings.go using the memory env vars as an example.

edit: we should be setting the default value in settings to not make it required imo***

@atoulme
Copy link
Contributor Author

atoulme commented Jul 28, 2023

Thanks! I was also looking for good ways to test this. I caught a few failing tests. I'll take any hint on what else we can add to test.

@rmfitzpatrick
Copy link
Contributor

rmfitzpatrick commented Jul 28, 2023

Thanks! I was also looking for good ways to test this. I caught a few failing tests. I'll take any hint on what else we can add to test.

The settings test suite has env var helpers to detect what's set by the collector: https://github.com/signalfx/splunk-otel-collector/blob/main/internal/settings/settings_test.go#L204.

We also have a default config test that should always reflect any changes: https://github.com/signalfx/splunk-otel-collector/blob/main/tests/general/default_config_test.go. I think it's good that it didn't require any changes though I'm not 100% on what the ":<port>" addr parsing semantics are atm.

edit: missed that you did actually change the integration tests**

@jeffreyc-splunk
Copy link
Contributor

We'll also need to add support to our deployments for the new env var when these changes are released.

@rmfitzpatrick rmfitzpatrick dismissed their stale review August 2, 2023 15:12

settings updated

@atoulme atoulme force-pushed the network_interface branch 2 times, most recently from 8ef36d2 to 0f36707 Compare August 3, 2023 02:10
CHANGELOG.md Outdated Show resolved Hide resolved
@atoulme
Copy link
Contributor Author

atoulme commented Aug 8, 2023

Going to squash to one commit and make sure the diff makes sense.

@atoulme
Copy link
Contributor Author

atoulme commented Aug 8, 2023

@jeffreyc-splunk please take a look when you have a moment. I am rerunning some of the ansible tests assuming that they're just flaky.

@jeffreyc-splunk
Copy link
Contributor

@jeffreyc-splunk please take a look when you have a moment. I am rerunning some of the ansible tests assuming that they're just flaky.

Yeah, unfortunately virtualbox/vagrant on the github runners is flaky. May take a couple of retries to get all of those tests to pass.

@jeffreyc-splunk
Copy link
Contributor

Not sure if it needs to be called out directly, but the new option in the installers will only be applicable for new installations or when the default unmodified config files are upgraded to this new version. If the customer uses a custom/modified config file, they will need to manually replace the values in their config with this env var for these changes to take effect.

@atoulme
Copy link
Contributor Author

atoulme commented Aug 9, 2023

I can maybe call that out in a subsequent PR if you don't mind, with a changelog entry that calls out to customers how to take advantage of this if they have an existing install.

@atoulme atoulme merged commit 5a6f688 into main Aug 10, 2023
355 checks passed
@delete-merged-branch delete-merged-branch bot deleted the network_interface branch August 10, 2023 00:47
@github-actions github-actions bot mentioned this pull request Jul 8, 2024
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.

4 participants