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

3/3 Extend Windows Performance Counters Receiver to support counters with instances #1229

Conversation

james-bebbington
Copy link
Member

@james-bebbington james-bebbington commented Oct 9, 2020

Link to tracking Issue: #1088

Description: Extend the Windows Performance Counters Receiver to support counters that includes "instances".

Testing: Coverage is not reported for code behind non-Linux build flags, but it is over 95% on Windows for this component:

> go test -coverprofile="coverage.txt" -covermode="atomic"
PASS
coverage: 96.5% of statements
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/windowsperfcountersreceiver  0.660s

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Oct 23, 2020
@bogdandrutu
Copy link
Member

Please rebase

@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #1229 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1229      +/-   ##
==========================================
+ Coverage   88.67%   88.68%   +0.01%     
==========================================
  Files         343      343              
  Lines       16753    16756       +3     
==========================================
+ Hits        14855    14860       +5     
+ Misses       1432     1431       -1     
+ Partials      466      465       -1     
Flag Coverage Δ
integration 70.87% <ø> (ø)
unit 87.32% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
receiver/windowsperfcountersreceiver/config.go 100.00% <100.00%> (ø)
receiver/k8sclusterreceiver/watcher.go 97.64% <0.00%> (+2.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65a4033...78664a3. Read the comment docs.

@james-bebbington james-bebbington force-pushed the windowsperfcounters-receiver-3 branch 2 times, most recently from f88ff1f to abe7a90 Compare October 31, 2020 22:07
@james-bebbington
Copy link
Member Author

All depencies have been merged and this is now ready for review

@james-bebbington james-bebbington force-pushed the windowsperfcounters-receiver-3 branch 2 times, most recently from 92d4a49 to d75c068 Compare November 2, 2020 03:37
receiver/windowsperfcountersreceiver/README.md Outdated Show resolved Hide resolved
receiver/windowsperfcountersreceiver/README.md Outdated Show resolved Hide resolved
Object string `mapstructure:"object"`
Counters []string `mapstructure:"counters"`
Object string `mapstructure:"object"`
Instances []string `mapstructure:"instances"`
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should consider being able to collect certain counters from only certain instances in the config? Or would one do this by using different receiver configs? For example, a case where one is interested in collecting counterA and counterB from instanceA only, and counterA and counterC from instanceB only.

Copy link
Member Author

@james-bebbington james-bebbington Nov 6, 2020

Choose a reason for hiding this comment

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

If I understand you correctly, this is supported, as per the example here and test here. You can achieve what you want as there's no limitation that you can only specify an Object once. Let me know if I missed something there.

Aside: there's currently no validation to stop you setting up completely duplicate counters, although I did leave a comment here mentioning we should consider adding such validation in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that's what I was referring to, thanks for pointing me to it!

@bogdandrutu bogdandrutu merged commit 76d1c75 into open-telemetry:master Nov 9, 2020
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
Add custom `markdown-link-check` to ignore 429 responses.
Diff with https://github.com/tcort/markdown-link-check/blob/v3.8.1/markdown-link-check:
```
< const markdownLinkCheck = require('markdown-link-check');
---
> const markdownLinkCheck = require('./');
134,138d133
<             // workaround to ignore 429 responses (too many requests)
<             if (result.statusCode === 429) {
<                 result.status = 'ignored'
<             }
<
```

**Link to tracking Issue:**

**Testing:**
Tested locally with `circleci` command line tool.

**Documentation:**
Updated .circleci/check-links/README.md.
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
* Move trace API to otel

* Move tracetest to oteltest

* Update package documentation

* Remove old api/trace package

* Lint

* Add changes to CHANGELOG

* Add tests for rest of trace API

* Apply suggestions from code review

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

* Documentation fixes

Includes resolutions for review issues.

* Correct CHANGELOG post release

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
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