-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
otelcol.Collector.DryRun() now instantiates all components #10031
Comments
As a workaround, I have switched to using the following function: func temporaryDryRunPatch(settings otelcol.CollectorSettings, ctxt context.Context) (err error) {
factories, err := settings.Factories()
if err != nil {
return fmt.Errorf("failed to initialize factories: %w", err)
}
confProv, err := otelcol.NewConfigProvider(settings.ConfigProviderSettings)
if err != nil {
return fmt.Errorf("failed to instantiate configuration provider: %w", err)
}
defer func() {
if shutdownErr := confProv.Shutdown(ctxt); shutdownErr != nil {
if err != nil {
err = fmt.Errorf("configuration-provider shutdown failed after encountering error: shutdown_err=%w, original_err=%w", shutdownErr, err)
} else {
err = fmt.Errorf("configuration-provider shutdown failed: %w", shutdownErr)
}
}
}()
cfg, err := confProv.Get(ctxt, factories)
if err != nil {
return fmt.Errorf("failed to get config: %w", err)
}
return cfg.Validate()
} This gives me the behavior I want, so this is not an especially high-priority issue. |
Thanks for reporting! Seems like this was caused by #9257 cc @djaglowski @ycombinator I don't quite understand why it happens though, my expectation would be that until we call
|
I'll work on a demonstration program. Having a day job is a drag. |
https://github.com/SeanPMiller/otelcol-issue-10031 This is essentially condensed from otelcorecol, except for the custom root command. |
Specific answers:
|
Thanks for the repro! I see how this can fail with the Prometheus endpoint, I am still a bit puzzled with the healthcheck case. |
I am, too. I'll see if I can reproduce, but it only seems to happen with a sufficiently complex configuration. |
I think this issue in contrib around |
That's to be expected, isn't it? The problem there IMO is that a component should not be opening any files during creation, only during startup.
Let's do this. It also seems like we have not set clear expectations on what components should do at creation time vs startup, so we should work on this as well. |
That may be true, and it is possible the k8s components have a bug, but the reason the user in that issue starting seeing an issue when using validate is because in #9257 instead of only calling |
I agree with reverting the change for now |
#### Description This PR reverts the change made in open-telemetry#9257 due to problems reported in open-telemetry#10031. <!-- Issue number if applicable --> #### Link to tracking issue Fixes open-telemetry#10031.
Describe the bug
This behavior might be intended. However, if you write your own entrypoint into the collector, as I have, and you call
otelcol.Collector.DryRun()
with a goal of determining whether the configuration "looks valid" before callingRun()
, thenservice.New()
is now called as part of the dry run, and its return value is discarded. This behavior is a consequence of this commit.Due to the above behavior, calling
DryRun()
now, for example, callsopen()
andbind()
for the self-monitoring telemetry-service port and the health-check extension port, among other ports. WhenRun()
is subsequently called, the second instance of each server component fails with abind()
error because its port has already been opened and bound.This might be a misuse of the
DryRun()
function. Let me know, and thanks in advance.Steps to reproduce
Write the above code.
What did you expect to see?
The old behavior.
What did you see instead?
What version did you use?
v0.98.0
Environment
go version go1.22.0 linux/amd64
The text was updated successfully, but these errors were encountered: