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

Integrate perflib #1241

Merged
merged 13 commits into from
Jul 19, 2023
Merged

Integrate perflib #1241

merged 13 commits into from
Jul 19, 2023

Conversation

jkroepke
Copy link
Member

@jkroepke jkroepke commented Jul 10, 2023

Fixes #1240

What I do here:

  1. Copy https://github.com/leoluk/perflib_exporter/tree/master/perflib to /perflib
  2. Copy needed consts from https://github.com/leoluk/perflib_exporter/blob/master/collector/mapper.go into /perflib/consts.go
  3. Remove unused helper
  4. Remove HelpName lookups, since windows_exporter do not use them
  5. Introduce lazy-loading for QueryNameTable to reduce startup times.
  6. Remove the additional loop at the QueryNameTable initialization.

@jkroepke
Copy link
Member Author

Implementations since from point of view, we may need some additional tests.

@jkroepke jkroepke marked this pull request as ready for review July 10, 2023 18:28
@jkroepke jkroepke requested a review from a team as a code owner July 10, 2023 18:28
@jkroepke
Copy link
Member Author

I run some test on Windows Server, I go no issues with default settings.

perflib/perflib.go Outdated Show resolved Hide resolved
@breed808
Copy link
Contributor

Looking at the commits the feature branch could do with a rebase (we're currently proposing to merge a merge commit); I have no objections to the change itself, it's looking good.

Once rebased, are you happy for me to merge this?

jkroepke and others added 12 commits July 15, 2023 09:59
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
@jkroepke
Copy link
Member Author

Once rebased, are you happy for me to merge this?

Let me take an other test, in case I do the rebase incorrectly.

@jkroepke
Copy link
Member Author

Everything seems working fine in default configuration.

image

@leoluk
Copy link

leoluk commented Jul 15, 2023

Are you interested in taking over perflib library maintenance? I no longer operate any Windows server and I'd be more than happy to archive the repo and point users here instead.

@jkroepke
Copy link
Member Author

@leoluk this PR contains only a subset of perflib_exporter. Only the functions and vars that windows_exporter requires are part of this PR

I guess to maintain the perflib_exporter, it requires an more deeper knowledge about the WMI metrics itself which I do not have.

Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkroepke are you happy for this to be merged? I have no further concerns or queries for this change.

@jkroepke
Copy link
Member Author

I'm happy, yes

@breed808 breed808 merged commit be9d287 into prometheus-community:master Jul 19, 2023
5 checks passed
@jkroepke jkroepke deleted the perflib branch July 19, 2023 09:38
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.

Integrate perflib_exporter into windows_exporter
3 participants