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

fix: support --remote-config startup option with Windows services #1443

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

fguimond
Copy link
Contributor

@fguimond fguimond commented Feb 9, 2024

With the introduction of remote management the application command line parameters were modified to include the --remote-config startup parameter. However this was unintentionally done only for interactive mode. Since the Windows service initialization code uses a different path the new parameter does not work correctly and the application exits.

The Windows service launch code path from the upstream repository uses the flags function defined in the upstream code, which doesn't support the --remote-config command line parameter. For interactive mode that function was redefined in our implementation of OtelCol to include the --remote-config command line parameter.

This PRs copies the upstream Windows service launch routine and modifies it slightly to use our version of the args function instead of the upstream version, making it possible to launch the OtelCol as a Windows server.

Testing has been performed by installing a MSI that includes this modified code. The Windows service now launches correctly as illustrated by the Sumo Logic console and Windows Service console.

image image

@fguimond fguimond requested a review from a team as a code owner February 9, 2024 19:49

// This is a copy of https://github.com/open-telemetry/opentelemetry-collector/blob/20710ff2aeaed3fe1c3e46423291522674db44d6/otelcol/collector_windows.go.
// This is required to maintain our command line flags when running the collector as a Windows service, until either
// the necessary names become public, or there's a better way of customizing config providers.
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like NewSvcHandler is public already, which seems to be the only thing we use in the patch, do we need other names/functions to be public? If so, maybe we could identify those and create an upstream issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! If windowsService was public in upstream we could simply redefine NewSvcHandler to use our flags function. None of the rest would need to be copied and things would be much cleaner indeed. Will create an upstream issue for it.

@fguimond fguimond merged commit 0d0239c into main Feb 12, 2024
47 checks passed
@fguimond fguimond deleted the dev-build/fix-support-remote-config-windows branch February 12, 2024 22:16
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.

2 participants