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

Adding Scheduled Tasks Collector #821

Merged
merged 9 commits into from
May 14, 2022
Merged

Adding Scheduled Tasks Collector #821

merged 9 commits into from
May 14, 2022

Conversation

mousavian
Copy link
Contributor

Adding a new collector called scheduled_task that can be enabled to provide metrics about registered tasks configured in the Windows Task Scheduler Service. ( #808 )

@mousavian mousavian requested a review from a team as a code owner July 17, 2021 16:19
@mousavian
Copy link
Contributor Author

Any idea why CI failed? Not sure why it's still trying to download go-ole v1.2.1 while this PR suggests higher version in mod file.

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.

This looks rather nice! I'll admit my knowledge of the Win32 API is lacking, so I'm not able to adequately review most of the getScheduledTasks() function. @carlpett could you take a look?
Documentation is also very good, thanks for putting in the effort for that 👍

You're right about the CI, I've been able to compile this PR locally with no issues. We may need to re-trigger the CI build to see if it's a repeatable error.

Copy link
Collaborator

@carlpett carlpett left a comment

Choose a reason for hiding this comment

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

Very nice! I have some questions on the OLE stuff, but generally 👍

var err error
scheduledTasks := []ScheduledTask{}

err = ole.CoInitialize(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As getScheduledTasks is called on each scrape, do we need to run initialization every time, or would it suffice to do once? I'm somewhat concerned about interactions with how the WMI library uses OLE as well, and especially with calling CoUninitialize.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, we need to consider concurrent scrapes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on my understanding and a little bit research on this:

  • The Go-OLE suggests to use CoInitializeEx function instead of coInitialize. The reason is explained here.
    On the other hand, the WMI library has already initialized the connection with COINIT_MULTITHREADED value, therefore we have to follow the same.

  • Microsoft website has explained here about how multiple initilizations and Uninitialization calls work, which is addressing your concern on the interaction of the existing WMI with this implementation:

    CoInitializeEx must be called at least once, and is usually called only once, for each thread that uses the COM library. Multiple calls to CoInitializeEx by the same thread are allowed as long as they pass the same concurrency flag, but subsequent valid calls return S_FALSE. To close the COM library gracefully on a thread, each successful call to CoInitialize or CoInitializeEx, including any call that returns S_FALSE, must be balanced by a corresponding call to CoUninitialize.

  • Locking OS thread to minimize the number of connections (possibly sharing with WMI) and preventing the "communication" and "connection" happening on different threads, if any.

  • do we need to run initialization every time? Good point, apparently not.
    The process which WMI has implemented includes creating and keeping the connection upon starting the application, and just running subsequent queries after that. I assume this is reliable enough(?).

I have updated the code according these.

collector/scheduled_task.go Outdated Show resolved Hide resolved
collector/scheduled_task.go Outdated Show resolved Hide resolved
collector/scheduled_task.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@carlpett carlpett left a comment

Choose a reason for hiding this comment

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

This looks very nice to me now, except one thing I missed earlier. With that fixed we'll be good to go :)

collector/scheduled_task.go Outdated Show resolved Hide resolved
mousavian added 7 commits May 12, 2022 16:36
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
@mousavian
Copy link
Contributor Author

mousavian commented May 12, 2022

I have rebased this... should be ready to merge :)

mousavian added 2 commits May 12, 2022 17:51
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Signed-off-by: Rahman Mousavian <rahman.mousavian@oracle.com>
Copy link
Collaborator

@carlpett carlpett left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@carlpett carlpett merged commit 3c4ae95 into prometheus-community:master May 14, 2022
@breed808
Copy link
Contributor

breed808 commented Jul 1, 2022

@Gerrit0284 this change hasn't yet made it into a new release yet; this was implemented after v0.18.1.
You can wait for a new release, or build the exporter from master.

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.

4 participants