fix: Enforce Singleton For Running Alloy Extension Instances#5763
fix: Enforce Singleton For Running Alloy Extension Instances#5763
Conversation
|
💻 Deploy preview available (fix: Enforce Singleton For Running Alloy Extension Instances): |
There was a problem hiding this comment.
Pull request overview
This PR enforces a singleton constraint for the alloyengine extension so only one instance can run per process, avoiding conflicts from process-global Default Engine state.
Changes:
- Add a process-wide atomic guard to prevent starting more than one
alloyengineextension instance. - Add a lifecycle test verifying a second instance fails to start while the first is running.
- Update user documentation to describe the limitation and clarify config path behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| extension/alloyengine/extension.go | Adds a process-global atomic singleton guard around Start() / run lifecycle. |
| extension/alloyengine/extension_test.go | Adds a test asserting the second instance fails while the first is running. |
| extension/alloyengine/README.md | Documents the new “single active instance per process” limitation. |
| docs/sources/set-up/otel_engine.md | Clarifies config can be file/dir and warns about the singleton limitation. |
e0b6240 to
b126b99
Compare
| } | ||
|
|
||
| func (e *alloyEngineExtension) Start(ctx context.Context, host component.Host) error { | ||
| func (e *alloyEngineExtension) Start(_ context.Context, host component.Host) error { |
There was a problem hiding this comment.
Should running be set to true in the beginning to avoid race conditions? Or we could use a mutex instead?
There was a problem hiding this comment.
running already uses an atomic.Bool type so it should be able to handle race conditions. But i'm i've implemented this in a way that will set this after flags have been parsed/validated so we don't set running to true unless we're actually running
There was a problem hiding this comment.
Ok, having an atomic bool is fine. It might be better to delete the defer statement from Start altogether? It'd make it easier to reason about edge cases.
For example:
- Is there a bug if there is already an alloy extension running that set "running" to true, and then the config is refreshed and startErr set to something non-nil causes "running" to become "false"?
- The "only one alloyengine extension can be active" error on line 118 is also technically a start error but startErr isn't set there.
There was a problem hiding this comment.
that's a good point, I think we can just do: 6619130
wdyt?
c24265f to
a6e9feb
Compare
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
Brief description of Pull Request
This addresses an issue brought up in #5669, where if you attempt to run multiple instances of the
alloyengineextension you will encounter errors. These errors are because the default engine has global state that gets mucked up when you try to run multiple pipelines in the same processPull Request Details
My proposal is: I don't think we should support this. We don't currently have a way of doing that outside of the extension, and if we enable the ability to run multiple default pipelines in one collector instance via the extension, it opens up for quite some complexity. Instead, I think we should do what extensions like
pprofdo and enforce a rule that only one extension instance can be started/running at once. I have stolen their implementation and applied it here to the alloy engine extension, and adjusted the documentation accordinglyThis change means that users can do something like
but as soon as they add an extension to their running list like
they will encounter an error with a clear indicator that only one extension can be running.
The other approach could be to merge the config for multiple extensions into one running extension - but this isn't very conceptually clean, and we already allow users to pass in a directory (since the extension just exposes the same functionality as the
runCLI command). I've made it a bit more clear in the docs here that you can indeed pass a directory through the extension, I feel that wasn't so clear before.