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

[receiver/windowsperfcountersreceiver] Create common package for windows performance counter receivers #9108

Merged
merged 23 commits into from
Apr 12, 2022

Conversation

Mrod1598
Copy link
Contributor

@Mrod1598 Mrod1598 commented Apr 7, 2022

Description:

Move common code of windows performance counter receiver into separate package to enable building of specific windows technology scrapers without needing to embed the generic windowsperfcounterreceiver into those other receivers.

Link to tracking Issue:
N/A

pkg/windowsperfcountercommon/Makefile Outdated Show resolved Hide resolved
extension/jaegerremotesampling/go.mod Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/scraper.go Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/config.go Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/scraper.go Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/scraper.go Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/scraper.go Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/scraper.go Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/scraper.go Outdated Show resolved Hide resolved
pkg/windowsperfcountercommon/scraper.go Outdated Show resolved Hide resolved
@Mrod1598 Mrod1598 requested a review from djaglowski April 8, 2022 13:19
pkg/winperfcounters/scraper.go Outdated Show resolved Hide resolved
pkg/winperfcounters/scraper.go Outdated Show resolved Hide resolved
pkg/winperfcounters/scraper.go Outdated Show resolved Hide resolved
pkg/winperfcounters/config.go Outdated Show resolved Hide resolved
pkg/winperfcounters/config.go Show resolved Hide resolved
@Mrod1598 Mrod1598 requested a review from djaglowski April 8, 2022 18:28
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Looks quite good now. A few minor things still.

pkg/winperfcounters/watcher.go Outdated Show resolved Hide resolved
pkg/winperfcounters/watcher.go Outdated Show resolved Hide resolved
pkg/winperfcounters/watcher.go Outdated Show resolved Hide resolved
@Mrod1598 Mrod1598 requested a review from djaglowski April 11, 2022 02:56
@Mrod1598 Mrod1598 marked this pull request as ready for review April 11, 2022 13:00
@Mrod1598 Mrod1598 requested a review from a team April 11, 2022 13:00
@Mrod1598 Mrod1598 requested a review from dashpole as a code owner April 11, 2022 13:00
pkg/winperfcounters/watcher.go Outdated Show resolved Hide resolved
now := pdata.NewTimestampFromTime(time.Now())
var errs error

metrics.EnsureCapacity(len(s.counters))

metricSlice.EnsureCapacity(len(s.watchers))
Copy link
Member

Choose a reason for hiding this comment

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

There is a problem here that we should address, but it can be a followup PR since it is not a regression.

We should not necessarily return a separate Metric instance for every counter, particularly when multiple counters represent data points that should belong to the same metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't necessarily make a new metric for every counter. The maximum amount of metrics there could be are the amount of watchers so the capacity is that much.

Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't aggregate data points onto the same metric instance when appropriate, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, it doesn't when it's generated metrics but does when you define the metric. I'll clean it up in a seperate PR.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM

@djaglowski djaglowski merged commit 681e98d into open-telemetry:main Apr 12, 2022
@Mrod1598 Mrod1598 deleted the windows-common-package branch April 12, 2022 13:48
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