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

Instrument obsreport.Scraper #6460

Conversation

moh-osman3
Copy link
Contributor

@moh-osman3 moh-osman3 commented Nov 2, 2022

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
obsreport/obsreporttest/otelprometheuschecker.go Outdated Show resolved Hide resolved
obsreport/obsreport_scraper.go Outdated Show resolved Hide resolved
@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
.chloggen/obsreport-scraper.yaml Outdated Show resolved Hide resolved
obsreport/obsreport_scraper.go Outdated Show resolved Hide resolved
@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
.chloggen/obsreport-scraper.yaml Outdated Show resolved Hide resolved
obsreport/obsreport_scraper.go Outdated Show resolved Hide resolved
obsreport/obsreport_scraper.go Outdated Show resolved Hide resolved

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

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.

obsreport/obsreport_scraper.go Show resolved Hide resolved

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

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
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