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
@atoulme atoulme force-pushed the network_interface branch from cfa0d5f to 38247e7 Compare July 28, 2023 07:51
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 atoulme force-pushed the network_interface branch from 0fd265b to a88b4f3 Compare August 7, 2023 23:15
@atoulme
Copy link
Contributor Author

atoulme commented Aug 8, 2023

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

@atoulme atoulme force-pushed the network_interface branch from f772ae1 to 12d0862 Compare August 8, 2023 19:01
@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.

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