windowsservicereceiver wireframe #35362

shalper2 · 2024-09-23T14:41:39Z

Description: wireframe and early pass at api for a windowsservices receiver as requested in issue 31377

Link to tracking Issue: 31377

Testing: none yet

Documentation: documentation around receiver scope in README.md

github-actions · 2024-10-11T05:20:55Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

pjanotti · 2024-10-23T18:28:32Z

I'll be reviewing this PR.

pjanotti

Some initial feedback for the wireframe

receiver/windowsservicereceiver/go.mod

pjanotti · 2024-10-29T22:59:59Z

receiver/windowsservicereceiver/config.go

+	scraperhelper.ControllerConfig `mapstructure:",squash"`
+	Services                       []string `mapstructure:"services"`    // user provided list of services to monitor with receiver
+	MonitorAll                     bool     `mapstructure:"monitor_all"` // monitor all services on host machine. supercedes services
+}


Assuming the default values for Services and MonitorAll it is not clear what would be the result: all services or none? It seems none giving that the boolean is false and the list of services is empty. Let me look at the rest of the code before making a suggestion here.

Looking at the previous art from telegraf: it seems reasonable to have a include and a exclude list of services. If the include is empty it assumes all services minus the exclude list.

receiver/windowsservicereceiver/windowsservice.go

receiver/windowsservicereceiver/factory.go

receiver/windowsservicereceiver/scraper.go

receiver/windowsservicereceiver/winapi.go

pjanotti · 2024-11-19T01:31:34Z

receiver/windowsservicereceiver/winapi.go

+}
+
+func (m *Manager) Disconnect() error {
+	return windows.CloseServiceHandle(m.Handle)


What happens if the Handle has the default value? Does it return an error?

It absolutely can return an error. obviously should be handled by the caller

Looking above I don't think this is an issue anymore with the current code: SCConnect will only succeed with a valid Handle.

pjanotti · 2024-11-19T01:32:55Z

receiver/windowsservicereceiver/windowsservice.go

+}
+
+func GetService(sname string) (*ServiceStatus, error) {
+	m, err := SCConnect()


We should connect at Start instead of on every call to GetService.

pjanotti · 2024-11-19T01:33:30Z

receiver/windowsservicereceiver/windowsservice.go

+
+func GetService(sname string) (*ServiceStatus, error) {
+	m, err := SCConnect()
+	defer m.Disconnect()


Typically you should add this defer only if connect succeeded.

pjanotti · 2024-11-19T01:44:04Z

receiver/windowsservicereceiver/windowsservice.go

+func (s *ServiceStatus) getStatus() error {
+	st, err := s.service.Query()
+	if err != nil {
+		return err


This is not a problem with the current code since the handle to the service is opened every time a GetService call is made. However, it is good to pay attention here that the value of s.ServiceStatus in case of error will be the default value for the type (0 in this case).

Similar in the call to Config below.

pjanotti · 2024-11-19T01:47:06Z

receiver/windowsservicereceiver/metadata.yaml

+    description: Gauge value containing service status as an integer value.
+    extended_documentation: >
+      Gauge values map to service status as follows:
+      1 - Stopped,


Suggested change

1 - Stopped,

0 - Failed to retrieve service status,

1 - Stopped,

pjanotti · 2025-01-13T21:05:54Z

.chloggen/31377-windowsservicereceiver-wireframe.yaml

+component: windowsservicereceiver
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: receiver for scraping windows service information from a host or properly configured remote machine.


Suggested change

note: receiver for scraping windows service information from a host or properly configured remote machine.

note: Introduce a new receiver to retrieve Windows services status information.

Initially is just the wire-frame and separating the support of remote that can be added later with the caveat that we should create a config that is compatible with later adding remote without breaking the initial config. Anyway, there is still more flexibility while the component is still in development.

pjanotti · 2025-01-13T21:09:30Z

receiver/windowsservicereceiver/documentation.md

+
+Gauge value containing service status as an integer value.
+
+Gauge values map to service status as follows: 0 - Failed to retrieve service status, 1 - Stopped, 2 - Start Pending, 3 - Stop Pending, 4 - Service Running, 5 - Continue Pending, 6 - Pause Pending, 7 - Service Paused


nit: it will be nice to have this as a list with each value and meaning on a single line, or perhaps row in a table.

pjanotti · 2025-01-13T21:10:43Z

receiver/windowsservicereceiver/factory_windows.go

@@ -0,0 +1,37 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+//go:build windows


nit: per convention add a blank line above

pjanotti · 2025-01-13T21:12:00Z

receiver/windowsservicereceiver/go.mod

+require (
+	github.com/google/go-cmp v0.6.0
+	github.com/stretchr/testify v1.9.0
+	go.opentelemetry.io/collector/component v0.114.0


After taking the comments versions should be updated to match the latest of the collector.

pjanotti · 2025-01-13T21:15:39Z

receiver/windowsservicereceiver/scraper.go

+	err := wss.mgr.Disconnect()
+	return err


Because there is no guarantee that start is called prior to calling shutdown, this should check if mgr is not nil:

Suggested change

err := wss.mgr.Disconnect()

return err

if wss.mgr == nil {

return nil

}

return wss.mgr.Disconnect()

pjanotti · 2025-01-13T21:18:30Z

receiver/windowsservicereceiver/winapi.go

+}
+
+func (m *Manager) Disconnect() error {
+	return windows.CloseServiceHandle(m.Handle)


Looking above I don't think this is an issue anymore with the current code: SCConnect will only succeed with a valid Handle.

MovieStoreGuy · 2025-01-24T01:47:05Z

There is no additional comments needed from me, @pjanotti has done an amazing job :)

If I can ask you to resolve comments as they are completed or addressed, and welcome to ping me to get this merged.

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch from bcf5660 to 6055531 Compare September 26, 2024 19:15

github-actions bot added the Stale label Oct 11, 2024

github-actions bot removed the Stale label Oct 24, 2024

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch 2 times, most recently from 5b0fb01 to c179923 Compare October 29, 2024 02:42

pjanotti reviewed Oct 31, 2024

View reviewed changes

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch from c179923 to 731526c Compare November 7, 2024 14:23

pjanotti mentioned this pull request Nov 8, 2024

Add Windows Service status metrics #31377

Open

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch 3 times, most recently from 6358dd5 to 0566f19 Compare November 13, 2024 17:33

pjanotti reviewed Nov 19, 2024

View reviewed changes

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch 3 times, most recently from a9bf88e to 7bc070a Compare December 3, 2024 15:02

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch 2 times, most recently from 7a65265 to 26077db Compare December 16, 2024 17:04

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch from 26077db to f934e59 Compare December 27, 2024 16:56

shalper2 added 10 commits January 8, 2025 10:02

chlog

823e2d4

initial wireframe

482176b

go mod tidy

982a9e6

added service metric

0be9dfd

first pass

f24edec

populate service struct

12963e5

lowered access on winapi syscalls

c8ca80e

make generate

12f4a70

added start, shutdown

480579b

factories

f6854c5

shalper2 added 2 commits January 8, 2025 10:03

scraperhelper

384e96f

make gotidy

4846d0c

shalper2 force-pushed the 31377-windowsservicereceiver-wireframe branch from f934e59 to 4846d0c Compare January 8, 2025 16:03

pjanotti reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windowsservicereceiver wireframe #35362

windowsservicereceiver wireframe #35362

shalper2 commented Sep 23, 2024

github-actions bot commented Oct 11, 2024

pjanotti commented Oct 23, 2024

pjanotti left a comment

pjanotti Oct 29, 2024

pjanotti Oct 31, 2024

pjanotti Nov 19, 2024

shalper2 Nov 19, 2024 •

edited

Loading

pjanotti Jan 13, 2025

pjanotti Nov 19, 2024

pjanotti Nov 19, 2024

pjanotti Nov 19, 2024

pjanotti Nov 19, 2024

pjanotti Nov 19, 2024

pjanotti Jan 13, 2025

pjanotti Jan 13, 2025

pjanotti Jan 13, 2025

pjanotti Jan 13, 2025

pjanotti Jan 13, 2025

pjanotti Jan 13, 2025

MovieStoreGuy commented Jan 24, 2025

	1 - Stopped,
	0 - Failed to retrieve service status,
	1 - Stopped,

	note: receiver for scraping windows service information from a host or properly configured remote machine.
	note: Introduce a new receiver to retrieve Windows services status information.


		Gauge value containing service status as an integer value.

		Gauge values map to service status as follows: 0 - Failed to retrieve service status, 1 - Stopped, 2 - Start Pending, 3 - Stop Pending, 4 - Service Running, 5 - Continue Pending, 6 - Pause Pending, 7 - Service Paused

-	err := wss.mgr.Disconnect()
-	return err
+	if wss.mgr == nil {
+		return nil
+	}
+	return wss.mgr.Disconnect()

windowsservicereceiver wireframe #35362

Are you sure you want to change the base?

windowsservicereceiver wireframe #35362

Conversation

shalper2 commented Sep 23, 2024

github-actions bot commented Oct 11, 2024

pjanotti commented Oct 23, 2024

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalper2 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy commented Jan 24, 2025

shalper2 Nov 19, 2024 •

edited

Loading