-
Notifications
You must be signed in to change notification settings - Fork 487
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
Integrations always running regardless of enabled flag #689
Conversation
pkg/integrations/manager.go
Outdated
@@ -280,6 +280,11 @@ func (m *Manager) ApplyConfig(cfg ManagerConfig) error { | |||
shouldCollect = *common.ScrapeIntegration | |||
} | |||
|
|||
// If Enabled is false then we should override the should collect | |||
if p.cfg.CommonConfig().Enabled == false { |
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.
Hmm, I don't think this is being done in the right place. The loop on line 210 would still spawn the integration, which it probably shouldn't be doing.
One way to handle this might be to add a function to return the list of only enabled integrations and iterate over that instead when calling ApplyConfig
. That way disabling an integration and removing it from the file are treated as the same.
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.
My thought on that was looking at the documentation is that enabled
flag enables the scraping but if you include the integration in the configuration with enabled = false
it still will get the statistics but won't send them. Should there be a difference in disabling an integration vs removing it, if the answer is no then I will put in my original fix. Specifically with the agent integration having it available but not sending for debugging purposes seemed useful. Can def go either way though.
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.
Nah, the intent is that enabled
drives whether the integration should be ran (so disabling it is the same as removing it), and scrape_integration
determines if that running integration should be scraped by the Agent automatically.
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.
Nice catch!
…t the config reloader so that it propagates from the source downstream
cmd/agent/main.go
Outdated
// Iterate over the configurations and remove any that Enabled = false, this works by setting i to the last element | ||
// and then returning the array minus the last element. This loses order but is performant. | ||
for i := 0; i < len(cfg.Integrations.Integrations);{ | ||
if !cfg.Integrations.Integrations[i].CommonConfig().Enabled { | ||
cfg.Integrations.Integrations[i] = cfg.Integrations.Integrations[len(cfg.Integrations.Integrations)-1] | ||
cfg.Integrations.Integrations = cfg.Integrations.Integrations[:len(cfg.Integrations.Integrations)-1] | ||
} else { | ||
i++ | ||
} | ||
} |
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.
Can you create a new function and call it from integrations.Manager.ApplyConfig instead? Filtering the actual slice here will omit the config from the results of /-/config
which might be a little confusing to users.
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.
This got more complicated. When you remove a configuration it did not stop the scraper, so when a configuration is disabled/removed and reload called Agent would list scrape_error = 404. Since the integration had stopped but the scraper was still running.
pkg/integrations/manager.go
Outdated
@@ -220,9 +222,14 @@ func (m *Manager) ApplyConfig(cfg ManagerConfig) error { | |||
continue | |||
} | |||
p.stop() | |||
m.im.DeleteConfig(key) |
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.
Beforehand the scraper was not stopping, but the integration was.
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'm not following how this is necessary, the second loop should be deleting the config?
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.
With the addition of the enabled check around line 227 the integration process is not started so the integration isn't added to the to m.integrations.
pkg/integrations/manager.go
Outdated
@@ -268,6 +279,7 @@ func (m *Manager) ApplyConfig(cfg ManagerConfig) error { | |||
|
|||
_ = m.im.DeleteConfig(key) | |||
process.stop() | |||
m.im.DeleteConfig(key) |
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.
Why do we need to call this twice?
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.
Not at all and removed.
pkg/integrations/manager.go
Outdated
if !ic.CommonConfig().Enabled { | ||
continue | ||
} |
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 still think a function to return a slice of enabled integrations would be easier to follow than having to maintain this check across the two loops, IMO.
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.
We still need the integrations in the array to disable/remove them, ie going from enabled -> disabled on reload.
pkg/integrations/manager.go
Outdated
@@ -220,9 +222,14 @@ func (m *Manager) ApplyConfig(cfg ManagerConfig) error { | |||
continue | |||
} | |||
p.stop() | |||
m.im.DeleteConfig(key) |
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'm not following how this is necessary, the second loop should be deleting the config?
Does this fix #686? That reports that set_collectors doesn't appear to be working with node_exporter, alongside this bug. |
It fixes the enable issue that cropped during #686 , I was unable to reproduce an actual issue with set collectors. I tested several variations and they behaved as expected. |
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.
Thanks! Can you add a regression test for this?
I'd also like to see a test that provides we need m.im.DeleteConfig in there twice, because it definitely seems unnecessary which makes it a candidate for accidental removal. Having a test would help prevent this.
Any updates on when this fix will be merged in? I am currently dealing with the fallout of this issue because the enabled flag isn't being honored. |
…so that it generated yaml.
Updated with unit tests and explanation below. Also had to update the mock integration so marshaling would work in the comparison. No tests were broken. Situation removal of config on line 225 in manager.go
If you run TestManager_IntegrationEnabledToDisabledReload with line 225 commented out, then the TestManager_IntegrationEnabledToDisabledReload will fail. |
As far as a release, currently unknown, it is going to be bundled with some other large changes. |
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.
LGTM after removing the duplicated if statement in the first loop
pkg/integrations/manager.go
Outdated
if !ic.CommonConfig().Enabled { | ||
continue | ||
} |
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.
Looks like this is in here twice :)
* Enabled flag should drive collection * Satisfy linter * Fix mock integration so it sets enabled = true by default * Make enabled the equivalent of not being in the config, removing it at the config reloader so that it propagates from the source downstream * Fix issue where removing the config/disabling did NOT stop the scraper * Remove whitespace * remove unneeded comment * Remove duplicated delete config * Update comments and add unit tests. Also had to fix mock integration so that it generated yaml. * Simplify unit test * change case because of refactor * filter integrations to enabled set * Move check up to the first statement * fix merge * Fix for double loop Co-authored-by: Robert Fratto <robertfratto@gmail.com>
PR Description
The manager is ALWAYS collecting the integration even if enabled is set to false.
Which issue(s) this PR fixes
Honor the enabled flag, when the manager was changed this behavior looks to have been lost.
Closes #686
Notes to the Reviewer
PR Checklist