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.

Sign up for GitHub

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

Jump to bottom

[receiver/hostmetrics] Migrate network scraper to new metrics builder #7048

Merged
merged 6 commits into from
Jan 21, 2022
Merged

[receiver/hostmetrics] Migrate network scraper to new metrics builder #7048

merged 6 commits into from
Jan 21, 2022

Conversation

abhide
Copy link
Member

@abhide abhide commented Jan 5, 2022 •
edited
Loading

Description:

Link to tracking Issue: open-telemetry/opentelemetry-collector#10904

Testing:

Documentation:

@linux-foundation-easycla
Copy link

CLA Not Signed

@abhide abhide force-pushed the GH-6429-nw-scraper branch 2 times, most recently from 2fbdbc2 to 457babf Compare January 7, 2022 08:00
@jpkrohling
Copy link
Member

@abhide , is there an issue tracking this? Do you plan on providing more info about this PR?

@abhide
Copy link
Member Author

abhide commented Jan 11, 2022

@abhide , is there an issue tracking this? Do you plan on providing more info about this PR?

@jpkrohling Yes, there is an issue tracking this: open-telemetry/opentelemetry-collector#10904
Yes, I will rebase and fix this PR

@abhide abhide force-pushed the GH-6429-nw-scraper branch from 457babf to bb8b05d Compare January 11, 2022 23:46
@abhide abhide marked this pull request as ready for review January 11, 2022 23:47
@abhide abhide requested a review from dmitryax as a code owner January 11, 2022 23:47
@abhide abhide requested a review from a team January 11, 2022 23:47
dmitryax
dmitryax reviewed Jan 13, 2022
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

One comment, otherwise good to me. Thanks for the contribution!

receiver/hostmetricsreceiver/testdata/config.yaml Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member

Two more things:

  • Please fix test failures: scrapeAndAppendNetworkCounterMetrics func don't need startTime argument anymore
  • Please add a line to Changelog Enhancements section

@abhide abhide force-pushed the GH-6429-nw-scraper branch from bb8b05d to 6685b23 Compare January 14, 2022 23:29
@dmitryax
Copy link
Member

@abhide build-and-test is still failing. also PTAL at my comments from the first review

@abhide
Copy link
Member Author

abhide commented Jan 19, 2022

@abhide build-and-test is still failing. also PTAL at my comments from the first review

Thanks for the review comments @dmitryax . I have fixed all comments except #7048 (comment)

@abhide abhide force-pushed the GH-6429-nw-scraper branch from 9e4dae3 to 75d9565 Compare January 19, 2022 07:44
dmitryax
dmitryax reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Just one nit, otherwise LGTM

receiver/hostmetricsreceiver/internal/scraper/networkscraper/network_scraper_test.go Show resolved Hide resolved
@bogdandrutu bogdandrutu requested a review from dmitryax January 20, 2022 20:24
dmitryax
dmitryax reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

One more comment, otherwise LGTM.

CHANGELOG.md Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member

And looks like tests are failing

@abhide abhide force-pushed the GH-6429-nw-scraper branch from 05e0925 to 0dcc770 Compare January 21, 2022 07:08
@abhide abhide force-pushed the GH-6429-nw-scraper branch from 0dcc770 to bee243b Compare January 21, 2022 07:12
@abhide
Copy link
Member Author

abhide commented Jan 21, 2022

And looks like tests are failing

Tests should pass now. I ran locally and make test is passing.

dmitryax
dmitryax reviewed Jan 21, 2022
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitryax dmitryax added the ready to merge Code review completed; ready to merge by maintainers label Jan 21, 2022
@bogdandrutu bogdandrutu merged commit 5ea0161 into open-telemetry:main Jan 21, 2022
@abhide abhide deleted the GH-6429-nw-scraper branch January 21, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitryax dmitryax dmitryax approved these changes

Assignees

@anuraaga anuraaga

Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abhide @jpkrohling @dmitryax @anuraaga @bogdandrutu