-
Notifications
You must be signed in to change notification settings - Fork 210
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
pkg/config: Set defaults for existing pprof configs again #4937
Conversation
✅ Meticulous spotted zero visual differences across 609 screens tested: view results. Expected differences? Click here. Last updated for commit 492376c. This comment will update as new commits are pushed. |
@alperkokmen you might want to test whether everything still works as expected after this merges. I suspect it should be fine, but just wanted to call it out since #4623 originally removed these defaults. |
Yes, I did test them locally with our scraper configuration. This is exactly what broke it, and now it's working again. I'll do some more testing once we have a container image. |
pkg/config/config.go
Outdated
@@ -205,6 +205,16 @@ func (c *ScrapeConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
unmarshalled.ProfilingConfig = defaults.ProfilingConfig | |||
} else if unmarshalled.ProfilingConfig.PprofConfig == nil { | |||
unmarshalled.ProfilingConfig.PprofConfig = defaults.ProfilingConfig.PprofConfig | |||
} else { | |||
// Merge unmarshalled config with defaults | |||
for pt := range unmarshalled.ProfilingConfig.PprofConfig { |
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.
Before this section was removed the code was like this.
for pt := range unmarshalled.ProfilingConfig.PprofConfig { | |
for pt := range defaults.ProfilingConfig.PprofConfig { | |
// nothing set yet so simply use the default | |
if unmarshalled.ProfilingConfig.PprofConfig[pt] == nil { | |
unmarshalled.ProfilingConfig.PprofConfig[pt] = pc | |
continue | |
} |
Maybe we want that again, but that means that we always add all defaults and then rather disable the ones we don't want?! I suppose that was the original behavior, though.
Somehow this code was removed. Because of that, if enabled or path aren't specified in the config they simply were left untouched and therefore weren't enabled or with the wrong path, / instead of /debug/pprof/...
Somehow this code was removed. Because of that, if enabled or path aren't specified in the config they simply were left untouched and therefore weren't enabled or with the wrong path, / instead of /debug/pprof/...
What the in-memory config looked like after unmarshalling before this fix:
What the in-memory config looks like after unmarshalling with this fix:
closes #4883