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

Update version of windows_exporter #5088

Closed

Conversation

MarkDordoy
Copy link

@MarkDordoy MarkDordoy commented Sep 5, 2023

PR Description

This PR updates the windows_exporter dependency to master (commit hash f8c298e038f8e0fcaf7f77fd591a5e1dfeb7f27b). Full change log here

Notes to the Reviewer

  • windows_ad_ldap_client_sessions added to ad collector. (@MarkDordoy)
  • windows_hyperv_host_cpu_wait_time_per_dispatch_total and windows_hyperv_vm_cpu_wait_time_per_dispatch_total added to hyperv collector. (@yuriyostapenko)
  • nps collector created. (@rebortg)
  • adfs collector metric docs updated to correctly reflect metric names. (@breed808)

PR Checklist

  • CHANGELOG.md updated

@MarkDordoy MarkDordoy requested a review from a team as a code owner September 5, 2023 09:34
@CLAassistant
Copy link

CLAassistant commented Sep 5, 2023

CLA assistant check
All committers have signed the CLA.

@MarkDordoy
Copy link
Author

I think this is ready for review. My first time contributing so please let me know if anything is missing/required and I'll get this updated.

@@ -134,7 +134,7 @@ require (
github.com/prometheus-community/elasticsearch_exporter v1.5.0
github.com/prometheus-community/postgres_exporter v0.11.1
github.com/prometheus-community/stackdriver_exporter v0.13.0
github.com/prometheus-community/windows_exporter v0.0.0-20230507104622-79781c6d75fc
github.com/prometheus-community/windows_exporter v0.23.2-0.20230904201428-f8c298e038f8
Copy link
Member

Choose a reason for hiding this comment

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

Apologies, this is a bit misleading. If you look a bit further down, you'll see a replace statement for this package.

We're actually using a fork of the Windows exporter over at https://github.com/grafana/windows_exporter to allow some config options to be propagated correctly.

It is a bit more toil, but for this exporter specifically we'd have to rebase our fork to a later tag of the upstream exporter, keep our required commits on top and update the replace statement to that SHA. cc @mattdurham

Copy link
Author

@MarkDordoy MarkDordoy Sep 5, 2023

Choose a reason for hiding this comment

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

ahh yes, I see now.

What would be the process to get this forked repo synced with master of the originating repo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Its on my todo, I have a PR to but it got stale. prometheus-community/windows_exporter#1227

will see if i can find some time to resolve

Copy link
Author

Choose a reason for hiding this comment

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

for my future reference @mattdurham , if I want to make changes/extend the windows_exporter, should I update the prometheus or grafana repo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Grafana though at some point we will get the changes merged upstream.

Copy link
Author

Choose a reason for hiding this comment

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

@mattdurham ok cool, for now I've added the update into the grafana fork, PR here: grafana/windows_exporter#21

if/when you merge that, I'll create a new PR to update the commit in the agent.

Thanks

CHANGELOG.md Outdated Show resolved Hide resolved
@MarkDordoy MarkDordoy force-pushed the md/update-windows-exporter branch from 7f8541d to 0459e98 Compare September 5, 2023 14:39
@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2023

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@github-actions github-actions bot added the needs-attention An issue or PR has been sitting around and needs attention. label Oct 8, 2023
@github-actions github-actions bot removed the needs-attention An issue or PR has been sitting around and needs attention. label Nov 4, 2023
@mattdurham
Copy link
Collaborator

closing this since we have updated windows exporter and no longer need the grafana fork

@mattdurham mattdurham closed this Jan 4, 2024
@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 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 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
Development

Successfully merging this pull request may close these issues.

4 participants