Fix default discovery crash on Windows #5202
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pjanotti
commented
Aug 8, 2024
samiura
reviewed
Aug 8, 2024
| args: []string{"otelcol", "--discovery", "--config=config/collector/agent_config.yaml"}, | ||
| timeout: 30 * time.Second, | ||
| }, | ||
| // Running the discovery with --dry-run in CI is not desirable because of the use of os.Exit(0) to end the execution. |
There was a problem hiding this comment.
--dry-run was originally intended as a helper/utility for the field/sales to capture what services are available but design requirements have changed. I wonder we need this test at all. But we can keep it like this. I do not have strong feelings either way.
There was a problem hiding this comment.
Got it, I will ask around to see if the need/purpose is clear.
samiura
reviewed
Aug 8, 2024
samiura
reviewed
Aug 8, 2024
samiura
approved these changes
Aug 8, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces #5162
Description:
A bad default on the OTel
docker_host(just fixed upstream, see open-telemetry/opentelemetry-collector-contrib#34358) exposed that if a observer extension fails toStartthe discovery code still callsListAndWatchwhat causes a panic.Details: the Start of the observer is called via internal/confmapprovider/discovery, but if Start fails it logs a warning and proceeds with the execution. Later internal/receiver/discoveryreceiver uses the host.GetExtensions() and the observer is there since it was created successfully (it did fail to start in a step that the host doesn't have visibility). The extension is cast to an observer.Observable and a go routine calling ListAndWatch is launched, but, then the private that it needs to run are nil because the start failed and then panic.
Right now I am removing the
docker_observerfrom Windows, but, it would be re-enabled as soon as we have it fixed on upstream.Jira:
https://splunk.atlassian.net/browse/OTL-2883
Testing:
Added 2 new tests and validated manually.
Documentation:
N/A