[windowsservicereceiver] Initial implementation#42545
Conversation
|
|
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
|
Het @merdzikdominik thanks for taking a look at this issue and contributing to it! Could you please include tests for the code that you are contributing? |
pjanotti
left a comment
There was a problem hiding this comment.
Thanks for working on this @merdzikdominik!
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //revive:disable:unused-parameter | ||
| //go:build windows |
There was a problem hiding this comment.
Since there is a file named factory_windows.go this file, factory.go, is expected to have code that is common to all platforms and this file should not need this compile directive.
There was a problem hiding this comment.
This directive shouldn't be necessary anymore... I see that it is still needed because there is still a NewFactory implementation in factory_others.go...
| ) | ||
| } | ||
|
|
||
| func createDefaultConfig() component.Config { |
There was a problem hiding this comment.
This same function is also defined in factory.go so there is no need to have it here, as long as the compile directive //go:build windows is removed from factory.go.
Side note: the suffix of the file names in go, e.g.: factory_windows.go, is in general enough for the compiler, but I personally like to have the compile directives there too.
|
|
||
| var errUnsupportedOS = errors.New("windowsservicereceiver: supported only on Windows") | ||
|
|
||
| func NewFactory() receiver.Factory { |
There was a problem hiding this comment.
At first I think this will be better if you have the same NewFactory at factory.go but then changing line 26 to receiver.WithMetrics(createMetricsReceiver, metadata.MetricsStability) and then implement the platform dependent createMetricsReceiver in factory_windows.go and factory_others.go. See receiver/windowsperfcountersreceiver for an example.
| continue | ||
| } | ||
|
|
||
| val := int64(mapStateToMetricValue(State(svc.status.State))) |
There was a problem hiding this comment.
Isn't possible to cast directly from svc.status.State?
|
Please follow https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md for new components - please create and link an issue here. |
replaced "get" prefixes with "update" for method names accordingly to their logic
(1) Errors are collected and returned collectively at the end (errsCh channel + multierr.Append). (2) Scrape is parallel (errgroup + concurrency-limiting semaphore). (3) Status is cast directly from svc.status.State with range validation. (4) Removed an unnecessary, unused bool from mapStartTypeToAttr; start has a clean signature. (5) Method names indicate the mutation: updateService, updateStatus, updateConfig.
(1) Errors are collected and returned collectively at the end (errsCh channel + multierr.Append). (2) Scrape is parallel (errgroup + concurrency-limiting semaphore). (3) Status is cast directly from svc.status.State with range validation. (4) Removed an unnecessary, unused bool from mapStartTypeToAttr; start has a clean signature. (5) Method names indicate the mutation: updateService, updateStatus, updateConfig.
…tor-contrib into merdzikdominik-windowsservicereceiver
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
pjanotti
left a comment
There was a problem hiding this comment.
If we get the file organization and API cleaned up I think we can move ahead. The next steps will be to:
- Add an integration test (select a server running on GH runners and check that we get the expected metrics)
- Refactor
serviceManager(likely remove it) so we can directly use themgr.Mgr(see comment)
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //revive:disable:unused-parameter | ||
| //go:build windows |
There was a problem hiding this comment.
This directive shouldn't be necessary anymore... I see that it is still needed because there is still a NewFactory implementation in factory_others.go...
|
@merdzikdominik Please, review the comments. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…nik-windowsservicereceiver-codereview2
…cereceiver-codereview2 Merdzikdominik windowsservicereceiver codereview2
…tor-contrib into merdzikdominik-windowsservicereceiver
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
|
Thank you for your contribution @merdzikdominik! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
Fully working windowsservicereceiver. Performed testing by running: - make test - make lint --------- Co-authored-by: Paulo Janotti <pjanotti@splunk.com>
Fully working windowsservicereceiver. Performed testing by running: - make test - make lint --------- Co-authored-by: Paulo Janotti <pjanotti@splunk.com>
Fully working windowsservicereceiver.
Performed testing by running: