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

pkg/config: Set defaults for existing pprof configs again #4937

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

metalmatze
Copy link
Member

@metalmatze metalmatze commented Aug 1, 2024

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:
config-before
What the in-memory config looks like after unmarshalling with this fix:
config-after

closes #4883

Copy link

alwaysmeticulous bot commented Aug 1, 2024

✅ 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.

@brancz
Copy link
Member

brancz commented Aug 1, 2024

@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.

@metalmatze
Copy link
Member Author

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.

@@ -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 {
Copy link
Member Author

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.

Suggested change
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/...
@metalmatze metalmatze merged commit 6b549ea into main Aug 5, 2024
38 checks passed
@metalmatze metalmatze deleted the config-fix branch August 5, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubernetes Pods not visible under targets section
2 participants