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

[opampsupervisor]: Skip executing the collector if no config is provided #35430

Conversation

BinaryFissionGames
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames commented Sep 26, 2024

Description:
If an empty config map is received, the supervisor does not run the agent.

The current logic here works fine, but I'm considering adding an option to only do this if the user opts into it. I'm not sure if there's a reason why a user might want to run the collector with the noop config though (maybe for the agent's self-telemetry?)

I've thought about it some more, and I don't think we need a config option here. If users want the collector to use a noop config, they can send a basic noop config.

I think we should also implement #32598 (closed as stale, we'll want to re-open this or open a new issue for it), which would allow users to configure a backup config to use when no config is provided by the server, if they would like.

Link to tracking Issue: Closes #33680

Testing:
e2e test added
Manually tested with a modified OpAMP server to send an empty config map

Documentation:
Update spec where it seemed applicable to call out this behavior.

@BinaryFissionGames BinaryFissionGames force-pushed the feat/opampsupervisor-start-stop-empty-confmap branch from 014352a to 7c00860 Compare October 1, 2024 07:57
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

One question but otherwise LGTM

cmd/opampsupervisor/supervisor/supervisor.go Outdated Show resolved Hide resolved
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

This all makes sense to me.

It's possible we'd want to surface some kind of error if the config is empty, but this behavior is more resilient, so I'm fine going with this until someone comes with a use-case that requires the behavior to be different.

@evan-bradley evan-bradley merged commit ea50558 into open-telemetry:main Oct 2, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 2, 2024
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this pull request Oct 4, 2024
…ded (open-telemetry#35430)

**Description:** <Describe what has changed.>
If an empty config map is received, the supervisor does not run the
agent.

~The current logic here works fine, but I'm considering adding an option
to only do this if the user opts into it. I'm not sure if there's a
reason why a user might want to run the collector with the noop config
though (maybe for the agent's self-telemetry?)~

I've thought about it some more, and I don't think we need a config
option here. If users want the collector to use a noop config, they can
send a basic noop config.

I think we should also implement open-telemetry#32598 (closed as stale, we'll want to
re-open this or open a new issue for it), which would allow users to
configure a backup config to use when no config is provided by the
server, if they would like.

**Link to tracking Issue:** Closes open-telemetry#33680

**Testing:**
e2e test added
Manually tested with a modified OpAMP server to send an empty config map

**Documentation:**
Update spec where it seemed applicable to call out this behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't execute otel collector if configuration is "noop"
4 participants