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

Add support for MSCluster collectors #885

Merged
merged 17 commits into from
Jun 23, 2022

Conversation

sstorie
Copy link

@sstorie sstorie commented Dec 6, 2021

Using the provided collector I added a few new collectors to grab information from Microsoft Clusters using the MSCluster WMI objects.

I'm not a Go programmer, so there's likely a more elegant way to handle this in a single file, but wanted to submit for some initial feedback.

@sstorie sstorie requested a review from a team as a code owner December 6, 2021 22:11
@sstorie
Copy link
Author

sstorie commented Dec 6, 2021

I meant to say "Using the provided collector-generator" in my initial comment.

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.

Thanks for submitting this!

I've briefly reviewed the changes and have left a few comments on the mscluster_network collector, which are applicable to the other collectors.

You'll also want to create entries for the collectors in the docs/ directory. Copying one of the existing markdown files might be easiest.

Let me know if any clarification or help is needed.

collector/mscluster_network.go Outdated Show resolved Hide resolved
collector/mscluster_network.go Outdated Show resolved Hide resolved
collector/mscluster_network.go Outdated Show resolved Hide resolved
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.

Thanks for the update Sam! I think you'll need to drop the commit with the go.mod and go.sum changes (or rebase your branch on master), as this has introduced a merge conflict. The cluster collectors aren't introducing new dependencies so the changes won't be required.

Once the metric & collector documentation has been added this should be good to merge 👍

@sstorie
Copy link
Author

sstorie commented Jan 4, 2022

Ok, haven't lost sight of this but with the US holidays I haven't had any time to spend here. I appreciate the feedback and will work to get you an update.

@sstorie sstorie force-pushed the mscluster branch 4 times, most recently from 85d39e7 to 22291fe Compare January 9, 2022 22:28
@sstorie sstorie marked this pull request as draft January 9, 2022 22:29
@sstorie
Copy link
Author

sstorie commented Jan 9, 2022

Thanks for the update Sam! I think you'll need to drop the commit with the go.mod and go.sum changes (or rebase your branch on master), as this has introduced a merge conflict. The cluster collectors aren't introducing new dependencies so the changes won't be required.

Once the metric & collector documentation has been added this should be good to merge 👍

I've got 2 of the 5 collectors updated with docs. Will keep plugging away over the next day or two and wrap this up.

@sstorie sstorie marked this pull request as ready for review January 10, 2022 02:35
@sstorie
Copy link
Author

sstorie commented Jan 10, 2022

I think I have everything in there and ready to go. Please let me know if you desire any other changes, or need the commits cleaned up somehow.

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.

Thanks Sam, this is looking good!

One last thing, could you add the cluster collectors as entries in the README.md collector list, with the documentation links?

@sstorie
Copy link
Author

sstorie commented Jan 11, 2022

Thanks Sam, this is looking good!

One last thing, could you add the cluster collectors as entries in the README.md collector list, with the documentation links?

Done.

@sstorie
Copy link
Author

sstorie commented Jan 17, 2022

Just to confirm - and I understand this may not be the highest priority - I am not aware of any other work being asked for here before accepting the merge. If there's something more expected on my end please let me know.

I may have caused an issue seeing or resolving the changes because of how I worked through the DCO process - my apologies if that's the case.

@breed808
Copy link
Contributor

Apologies for the delay Sam, I've been pretty busy with work recently and haven't been able to get back to this.

The CI test job is failing due to a bug in the build tool installation command, which I've raised a PR to fix.
Once that's merged I'll re-run the CI for this PR and get it merged.

@breed808
Copy link
Contributor

@sstorie could you rebase this PR on master and push? The CI fix has been merged but needs to be present in your feature branch.

@jpatigny
Copy link

hey guys, any update on this ?
I'd love to have this feature.

@sstorie would you be able to do the rebase or do you want someone else to care of it ?

@breed808
Copy link
Contributor

I rebased and force pushed the branch. @sstorie if you have any local changes, rebase them onto master and force push to overwrite my changes.

@sstorie
Copy link
Author

sstorie commented Jun 19, 2022

No I don't have any local changes, and thank you for taking the initiative to get this pulled in.

@breed808
Copy link
Contributor

The DCO check isn't happy, likely due to the nature of the rebase. I'll force a merge as all other checks have passed.

@breed808 breed808 merged commit 93dcdf9 into prometheus-community:master Jun 23, 2022
@sstorie sstorie deleted the mscluster branch June 23, 2022 11:21
anubhavg-icpl pushed a commit to anubhavg-icpl/windows_exporter that referenced this pull request Sep 22, 2024
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.

3 participants