Add ScrapeErrors struct to simplify errors usage #2414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #2414 +/- ##
==========================================
- Coverage 91.78% 91.76% -0.02%
==========================================
Files 266 266
Lines 15099 15090 -9
==========================================
- Hits 13858 13847 -11
- Misses 865 866 +1
- Partials 376 377 +1
Continue to review full report at Codecov.
|
ozerovandrei
force-pushed
the
2046-improve-part-scrape-err
branch
from
655c573 to
37f7da5
Compare
ozerovandrei
force-pushed
the
2046-improve-part-scrape-err
branch
from
37f7da5 to
8c83cd7
Compare
bogdandrutu
reviewed
Feb 3, 2021
ozerovandrei
force-pushed
the
2046-improve-part-scrape-err
branch
from
8c83cd7 to
af87ba0
Compare
bogdandrutu
reviewed
Feb 7, 2021
ozerovandrei
force-pushed
the
2046-improve-part-scrape-err
branch
4 times, most recently
from
97aab26 to
cc49843
Compare
|
Rebased and applied changes from 5cc0707. |
bogdandrutu
reviewed
Feb 16, 2021
| } | ||
|
|
||
| // Add adds a PartialScrapeError with the provided failed count and error. | ||
| func (s *ScrapeErrors) Add(failed int, err error) { |
There was a problem hiding this comment.
I would call this AddPartial, and rename AddRegular to just Add?
Comment on lines
45
to
47
| func (s *ScrapeErrors) AddRegularf(format string, a ...interface{}) { | ||
| s.errs = append(s.errs, fmt.Errorf(format, a...)) | ||
| } |
There was a problem hiding this comment.
Is this too much? Users can simply call AddRegular(fmt.Errorf(format, ...))
|
|
||
| // Addf adds a PartialScrapeError with the provided failed count and arguments to format an error. | ||
| func (s *ScrapeErrors) Addf(failed int, format string, a ...interface{}) { | ||
| s.errs = append(s.errs, NewPartialScrapeError(fmt.Errorf(format, a...), failed)) |
There was a problem hiding this comment.
Do we need PartialScrapeError to be exposed anymore? We should not remove it in this PR, but want to make sure we have a plan either remove it in a followup, or keep it there and understand why is necessary.
There was a problem hiding this comment.
We use Failed field of PartialScrapeError in other packages:
https://github.com/open-telemetry/opentelemetry-collector/blob/main/obsreport/obsreport_scraper.go#L104
So we need to introduce a method to retrieve this value and maybe add an interface instead of the exported PartialScrapeError struct.
Add ScrapeErrors that contains a slice with errors and that has methods to simplify adding new PartialScrapeErrors and regular generic errors. Use new methods to refactor errors appends in receiver/hostmetricsreceiver. Use ScrapeErrors.Combine() in component/componenterror to not create cycling dependencies in consumererrors package.
Leave componenterror.CombineErrors unchanged to not introduce too many changes in a single PR. Add a note about the necessity of componenterror.CombineErrors deprecation.
Rename scrapeErrsCount field to failedScrapeCount. Add failed count in every Add, Addf call of ScrapeErrors and use this info to call CombineErrors directly instead of iterating over all errors in ScrapeErrors. Fix filesystemscraper Scrape function to not cast error into PartialScrapeError since it will cause panics after combining ScrapeErrors. Use NewPartialScrapeError instead.
Cast getProcessMetadata() error into consumererror.PartialScrapeError to get the valid scrape errors count.
Cast Scrape() error into consumererror.PartialScrapeError to get the valid scrape errors count and to not skip combined errors.
Fix `Combine()` method to check if errs slice contains `PartialScrapeError` since we can have scrape errors with 0 failed metrics so checking only `failedScrapeCount` field is not enough.
Do not change comments in consumer/consumererror/combineerrors_test.go.
Apply 5cc0707 to component/componenterror.
Use only Add and AddPartial methods.
ozerovandrei
force-pushed
the
2046-improve-part-scrape-err
branch
from
cc49843 to
0c4c329
Compare
bogdandrutu
approved these changes
Feb 19, 2021
bogdandrutu
approved these changes
Feb 19, 2021
This was referenced Mar 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description: Add ScrapeErrors that contains a slice with errors and that has methods
to simplify adding new PartialScrapeErrors and regular generic errors.
Use new methods to refactor errors appends in
receiver/hostmetricsreceiver.
Use ScrapeErrors.Combine() in component/componenterror to not create
cycling dependencies in consumererrors package.
Link to tracking Issue: #2046