-
Notifications
You must be signed in to change notification settings - Fork 143
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
Propagate apm config #3223
Propagate apm config #3223
Conversation
This pull request does not have a backport label. Could you fix it @pchila? 🙏
NOTE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is in draft, but want to provide some early feedback. I like how this is looking.
This pull request is now in conflicts. Could you fix it? 🙏
|
8e57dc8
to
b7960e6
Compare
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing serious, but I believe you still need to update to a released version of the elastic-agent-client
and there was an append
that was doing nothing. So either you forgot to add the element to append, or the line should be removed
This pull request is now in conflicts. Could you fix it? 🙏
|
62e927c
to
9283429
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if that kind of doc already exists for other configs, but how about documenting how services should handle the added config?
type ConfigPatch func(change ConfigChange) ConfigChange | ||
|
||
// ConfigPatchManager is a decorator to restore some agent settings from the elastic agent configuration file | ||
type ConfigPatchManager struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this struct is lacking tests?
"github.com/elastic/elastic-agent/pkg/utils" | ||
) | ||
|
||
func InjectAPMConfig(comps []component.Component, cfg map[string]interface{}) ([]component.Component, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone who doesn't know much/anything about elastic-agent, I find it hard to understand why we have to inject and then patch. How about documenting those two public methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add some documentation, the short version is:
PatchAPMConfig
will patch agent configuration coming from fleet (which does not support APM configuration at the moment) by adding the APM config values retrieved from the elastic-agent.yaml fileInjectAPMConfig
will add APM configuration to the internal datamodel agent uses to determine which components and units to run
} | ||
|
||
func (c ConfigPatchManager) Run(ctx context.Context) error { | ||
go c.patch(c.inner.Watch(), c.outCh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unchecked goroutine here feels a bit brittle/unsafe. Could we end up with an explosion of running goroutines? Should there be a timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you mean an unchecked goroutine? Are you referring to the fact that there is no waitgroup or other syncronization object for the goroutine ?
This structure will decorate an inner ConfigManager
and patch configuration using the goroutine you see here for as long as the inner ConfigManager is running (the lifespan of the goroutine depends on the inner configmanager lifespan)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If somehow, inner.Run()
returns right away, but inner.Watch()
doesn't at all, then you could end up with a goroutines leak.
I also don't have much context on this code at all, so maybe that's not really possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inner.Watch()
returns the channel on which config changes are published. We use that channel to intercept configuration changes that we want to patch.
The basic pattern for a ConfigManager is to start it and then consume all the configuration changes passed on the channel.
The ConfigPatchManager does the same thing with the addition of a goroutine that will read from the inner channel, patch and publish on its own channel
🌐 Coverage report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really looked through the code. It is very well documented and has a good amount of tests. I really like the addition in the fake component to really test this change from Elastic Agent down to the actual running component.
/test |
buildkite test this |
This feature is for internal use at the moment, so no official docs are created at this stage. Regarding how services should handle the added config: it depends :)
As of now the agent and the agent client will not set the APM env variables for the component or restart it, the processing is delegated to the component itself (if then the component decides that setting the env and reexecuting is the easier way to apply the configuration, that is obviously an option) |
buildkite test this |
1 similar comment
buildkite test this |
874af15
to
fff955e
Compare
SonarQube Quality Gate |
What does this PR do?
This change propagates the APM configuration set up for agent to the components that are managed.
We only support Elastic APM at the moment and at the time of this change this can only be configured in
elastic-agent.yml
configuration file.For fleet-managed agents we include a workaround that will inject this configuration from the config file; this workaround inject the configuration from the config file in all the config changes received from Fleet, however it does not support hot reloading in this configuration: any changes to the apm configuration will take effect after a restart.
Here's what an example of APM configuration looks like:
Why is it important?
We want to manage the apm configuration from a single place for all components run via Elastic Agent.
Checklist
[ ] I have added an entry in./changelog/fragments
using the changelog toolAuthor's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs
Questions to ask yourself