Skip to content

Conversation

@SuperQ
Copy link
Member

@SuperQ SuperQ commented Jan 28, 2023

Move metric descriptiions to package vars to avoid allocating them every time NewCPUFreqCollector() is called.

Signed-off-by: Ben Kochie [email protected]

Move metric descriptiions to package vars to avoid allocating them every
time `NewCPUFreqCollector()` is called.

Signed-off-by: Ben Kochie <[email protected]>
@SuperQ SuperQ requested a review from discordianfish January 28, 2023 10:44
@SuperQ
Copy link
Member Author

SuperQ commented Jan 28, 2023

CC @britcey, I don't think this will help your situation, unless you're using the collect[] URL param.

@SuperQ SuperQ requested a review from Nexucis February 25, 2023 17:41
@SuperQ SuperQ merged commit c914f00 into master Feb 25, 2023
@SuperQ SuperQ deleted the superq/cpufreq_common branch February 25, 2023 19:18
@dswarbrick
Copy link
Contributor

Isn't NewCPUFreqCollector() only called once at startup, i.e. by NewNodeCollector() calling the various collectors' factory functions?

I don't really see how this is any significant optimization.

@discordianfish
Copy link
Member

@SuperQ hrmm @dswarbrick is right, why this change?

@SuperQ
Copy link
Member Author

SuperQ commented Mar 7, 2023

No, this can be called on every scrape if you call it with the collect[] URL param.

@discordianfish
Copy link
Member

Ah I see.. maybe then we should go through all collectors to look for similiar saving.

@dswarbrick
Copy link
Contributor

Ah I see.. maybe then we should go through all collectors to look for similiar saving.

Oof. That's going to result in a LOT of package-level variables pollution.

@discordianfish
Copy link
Member

@dswarbrick Yeah I'm worrying about that as well. I wanted to refactor the collector package for a while and move related functionality to subpackages but since nobody is paying me currently for anything prometheus related, I won't get to that anytime soon..

@SuperQ
Copy link
Member Author

SuperQ commented Mar 7, 2023

Eh, I don't see the package vars for the collector Descs as a big deal. I think we should migrate collectors to this new pattern. It's not a huge performance change for most users, but it would be good to keep things consistent.

We actually discussed this pattern as part of our refactoring the postgres_exporter to use a similar collector package. We decided to go with package level Desc vars to avoid any reallocaiton.

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.

6 participants