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

[Chore] Migration error and helper into scraper model #11166

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

zzzk1
Copy link
Contributor

@zzzk1 zzzk1 commented Sep 13, 2024

Description

Move helper and error packages into scraper module.

Link to tracking issue

Fixes #11003

Testing

Documentation

@zzzk1
Copy link
Contributor Author

zzzk1 commented Sep 13, 2024

cc @mx-psi plta.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This is a good start, I have two comments:

  1. Can we keep the original names (scraperhelper, scrapererror)? When importing, Go will only use the package name, so we tend to repeat the parent package name if the name is not specific
  2. To help in the transition, we should keep the original scraper packages for one release as documented here: https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#breaking-changes. To do this, you can have a type alias for each symbol like this:
type ScrapeFunc = scraperhelper.ScrapeFunc

We then can deprecate the whole package as mentioned here https://go.dev/wiki/Deprecated#examples

receiver/scraper/error/doc.go Outdated Show resolved Hide resolved
receiver/scraper/helper/config.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 95.96413% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.86%. Comparing base (fed6dfe) to head (b9b6763).
Report is 111 commits behind head on main.

Files with missing lines Patch % Lines
...eceiver/scraper/scraperhelper/scrapercontroller.go 95.00% 3 Missing and 2 partials ⚠️
receiver/scraper/scraperhelper/obsreport.go 95.12% 1 Missing and 1 partial ⚠️
receiver/scraper/scraperhelper/scraper.go 91.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11166      +/-   ##
==========================================
+ Coverage   91.79%   91.86%   +0.06%     
==========================================
  Files         432      438       +6     
  Lines       20426    20645     +219     
==========================================
+ Hits        18751    18965     +214     
- Misses       1301     1302       +1     
- Partials      374      378       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mx-psi
Copy link
Member

mx-psi commented Oct 4, 2024

Looks like this needs a make gotidy

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Can you try to make this to preserve history. in the old packages (to simplify migration) would be easier to make the old types/funcs aliases of the new one.

@@ -0,0 +1 @@
include ../../Makefile.Common
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
include ../../Makefile.Common
include ../../Makefile.Common

Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Oct 29, 2024
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver] Move scraper packages into scraper module
3 participants