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

Switch to upstream windows_exporter #3603

Merged
merged 5 commits into from
May 9, 2023

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Apr 23, 2023

PR Description

This PR removes the outdated internal grafana/windows_exporter fork.

This PR is still in draft, since various upstream PR needs to be solved first. This PR based on personal forks of windows_exporter which are planned to be merged upstreamed. After that, the PR is ready to review.

I'm creating the PR right now to indicate that some is working on the windows_exporter.

It introduce a breaking change from prometheus-community/windows_exporter#1170 (not sure how to handle this). If Grafana Agent follows SemVer, the breaking change is fine.

Test on a windows 10 system with following config:

prometheus.exporter.windows "exporter1" {
  // Subset of overall collectors  
  enabled_collectors = "cpu,cs,logical_disk"
}

prometheus.exporter.windows "exporter2" {
  // Different subset of overall collectors  
  enabled_collectors = "net,os,service,system"
}

prometheus.scrape "example" {
  targets = concat(
    prometheus.exporter.windows.exporter1.targets,
    prometheus.exporter.windows.exporter2.targets,
  )

  forward_to = [...]
}

Prometheus UI:

image

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@jkroepke jkroepke changed the title Use upstream windows_exporter Switch to upstream windows_exporter Apr 23, 2023
@jkroepke jkroepke force-pushed the update_windows_exporter branch 3 times, most recently from 221ce36 to a2d0899 Compare April 23, 2023 23:14
@jkroepke jkroepke force-pushed the update_windows_exporter branch 11 times, most recently from b6abe5c to ec6af39 Compare May 7, 2023 17:49
@jkroepke jkroepke marked this pull request as ready for review May 7, 2023 17:49
@mattdurham
Copy link
Collaborator

Reviewing this and going to run it on my local VM to smoke test it a bit.

@mattdurham
Copy link
Collaborator

mattdurham commented May 8, 2023

Not your issue but can we fix the doc, this should be default in the example

@jkroepke
Copy link
Contributor Author

jkroepke commented May 8, 2023

@mattdurham I hope I address all you issues!

@jkroepke jkroepke requested a review from mattdurham May 8, 2023 20:35
@mattdurham
Copy link
Collaborator

I may tweak a few things here and there but this is solid.

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

Fantastic work, some small formatting things/deprecation notes but I will fix them post merge.

@mattdurham mattdurham merged commit 59d22c9 into grafana:main May 9, 2023
@jkroepke jkroepke deleted the update_windows_exporter branch May 9, 2023 13:21
@mattdurham
Copy link
Collaborator

@jkroepke can you debug the windows_exporter, I noticed when debugging agent on windows and also windows_exporter directly I get a syscall error. Several others were able to recreate. Running without the debugger is fine.

@mattdurham
Copy link
Collaborator

Seems to be a specific line in perflib.go syscall.RegCloseKey(syscall.HKEY_PERFORMANCE_DATA), if debugger is attached it fails, if not then it succeeds

@mattdurham
Copy link
Collaborator

And to close this out found the issue leoluk/perflib_exporter#42, if you check the error while running on RegKeyClose it returns invalid handle which for whatever reason crashes when debugger is attached.

@kago-dk
Copy link

kago-dk commented Jul 8, 2023

https://github.com/grafana/agent/blob/main/docs/sources/static/configuration/integrations/windows-exporter-config.md should be updated to cover the changes mentioned in the changelog.

And the changelog states iss instead of iis
image

clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
* Integrate new windows_exporter

* go mod tidy

* Add Black and whitelist to initial config

* fix ordering

* fix docs
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
* Integrate new windows_exporter

* go mod tidy

* Add Black and whitelist to initial config

* fix ordering

* fix docs
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
3 participants