Skip to content

Instrument obsreport.Scraper #6460

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
Merged
merged 11 commits into from
Nov 17, 2022
Merged

Instrument obsreport.Scraper #6460

merged 11 commits into from
Nov 17, 2022

Conversation

@moh-osman3
Copy link
Contributor

@moh-osman3 moh-osman3 commented Nov 2, 2022 •
edited
Loading

Description:

This PR instruments obsreport.Scraper with otel go. This is a followup PR that is based on similar changes made for obsreport.Receiver: #6222

Link to tracking Issue: Part of #816

Testing:
Ran contrib collector with prometheusreceiver and hostmetricsreceiver and confirmed that new metrics show up. Tested both with feature gate disabled and enabled.

curl http://localhost:8888/metrics | tail -n 13
# HELP otelcol_scraper_errored_metric_points_ratio_total Number of metric points that were unable to be scraped.
# TYPE otelcol_scraper_errored_metric_points_ratio_total counter
otelcol_scraper_errored_metric_points_ratio_total{receiver="hostmetrics",scraper="memory"} 0
# HELP otelcol_scraper_errored_metric_points_ratio_total_total Number of metric points that were unable to be scraped.
# TYPE otelcol_scraper_errored_metric_points_ratio_total_total counter
otelcol_scraper_errored_metric_points_ratio_total_total{receiver="hostmetrics",scraper="cpu"} 4
# HELP otelcol_scraper_scraped_metric_points_ratio_total Number of metric points successfully scraped.
# TYPE otelcol_scraper_scraped_metric_points_ratio_total counter
otelcol_scraper_scraped_metric_points_ratio_total{receiver="hostmetrics",scraper="cpu"} 0
# HELP otelcol_scraper_scraped_metric_points_ratio_total_total Number of metric points successfully scraped.
# TYPE otelcol_scraper_scraped_metric_points_ratio_total_total counter
otelcol_scraper_scraped_metric_points_ratio_total_total{receiver="hostmetrics",scraper="memory"} 2
# HELP otelcol_target_info Target metadata

@moh-osman3 moh-osman3 requested review from a team and Aneurysm9 November 2, 2022 02:28
@moh-osman3 moh-osman3 force-pushed the mohosman/instrument-obsreport-scraper-and-test branch from 3b37e8f to 45a051a Compare November 3, 2022 17:27
paivagustavo
paivagustavo reviewed Nov 3, 2022
View reviewed changes
@moh-osman3 moh-osman3 changed the title Instrument obsreport.Scraper (#19) Instrument obsreport.Scraper Nov 3, 2022
@moh-osman3 moh-osman3 force-pushed the mohosman/instrument-obsreport-scraper-and-test branch 2 times, most recently from bdc7135 to 3436c45 Compare November 9, 2022 18:36
bogdandrutu
bogdandrutu reviewed Nov 14, 2022
View reviewed changes
@moh-osman3 moh-osman3 force-pushed the mohosman/instrument-obsreport-scraper-and-test branch from adb6a40 to 238e140 Compare November 15, 2022 03:13
@moh-osman3 moh-osman3 requested review from bogdandrutu and removed request for Aneurysm9 November 15, 2022 17:12
bogdandrutu
bogdandrutu reviewed Nov 15, 2022
View reviewed changes
bogdandrutu
bogdandrutu approved these changes Nov 15, 2022
View reviewed changes
obsreport/obsreport_test.go Outdated

parentCtx, parentSpan := tt.TracerProvider.Tracer("test").Start(context.Background(), t.Name())
defer parentSpan.End()
testTelemetry(t, func(tt obsreporttest.TestTelemetry, registry *featuregate.Registry) {
Copy link
Member

Choose a reason for hiding this comment

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

This API is horrible, makes everything be super indented...

/cc @paivagustavo

Copy link
Member

Choose a reason for hiding this comment

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

@jmacd suggested the creation of new top level function tests as 5961f60, which keeps the same indentation for the existing tests and can be easily removed once we're done with the transition.

cc @moh-osman3

Copy link
Contributor

Choose a reason for hiding this comment

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

Please refactor as suggested, and we can get this finalized + merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if my latest commit matches what was suggested.

@codecov
Copy link

codecov bot commented Nov 17, 2022 •
edited
Loading

Codecov Report

Base: 91.33% // Head: 90.96% // Decreases project coverage by -0.36% ⚠️

Coverage data is based on head (ee51526) compared to base (375a859).
Patch coverage: 85.71% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6460      +/-   ##
==========================================
- Coverage   91.33%   90.96%   -0.37%     
==========================================
  Files         242      241       -1     
  Lines       13888    14063     +175     
==========================================
+ Hits        12684    12792     +108     
- Misses        959     1019      +60     
- Partials      245      252       +7
Impacted Files Coverage Δ
internal/obsreportconfig/obsreportconfig.go 94.17% <75.00%> (-2.74%) ⬇️
obsreport/obsreport_scraper.go 91.30% <84.90%> (+1.72%) ⬆️
obsreport/obsreporttest/obsreporttest.go 83.62% <100.00%> (-3.48%) ⬇️
obsreport/obsreporttest/otelprometheuschecker.go 94.05% <100.00%> (+0.65%) ⬆️
consumer/consumertest/base_consumer.go 0.00% <0.00%> (-100.00%) ⬇️
component/config.go 0.00% <0.00%> (-40.00%) ⬇️
component/component.go 62.22% <0.00%> (-37.78%) ⬇️
component/componenttest/nop_factories.go 0.00% <0.00%> (-29.42%) ⬇️
processor/batchprocessor/metrics.go 84.71% <0.00%> (-15.29%) ⬇️
component/extension.go 86.66% <0.00%> (-13.34%) ⬇️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

codeboten
codeboten reviewed Nov 17, 2022
View reviewed changes
obsreport/obsreport_test.go Outdated

parentCtx, parentSpan := tt.TracerProvider.Tracer("test").Start(context.Background(), t.Name())
defer parentSpan.End()
testTelemetry(t, func(tt obsreporttest.TestTelemetry, registry *featuregate.Registry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refactor as suggested, and we can get this finalized + merged

@moh-osman3 moh-osman3 requested a review from codeboten November 17, 2022 18:29
codeboten
codeboten approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @moh-osman3!

@bogdandrutu bogdandrutu merged commit 4565692 into open-telemetry:main Nov 17, 2022
@moh-osman3 moh-osman3 mentioned this pull request Aug 15, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codeboten codeboten codeboten approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

+1 more reviewer

@paivagustavo paivagustavo paivagustavo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@moh-osman3 @codeboten @bogdandrutu @paivagustavo