[chore][receiver/splunkenterprise]Splunkent scrape refactor #31184
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
shalper2
force-pushed
the
splunkent-scrape-refactor
branch
3 times, most recently
from
ec46db8
to
e2bc83f
Compare
shalper2
force-pushed
the
splunkent-scrape-refactor
branch
from
e2bc83f
to
07481d5
Compare
greatestusername-splunk
approved these changes
Mar 18, 2024
Comment on lines
111
to
130
| go func() { | ||
| errorListener(ctx, errChan, errOut) | ||
| }() | ||
|
|
||
| for _, fn := range metricScrapes { | ||
| wg.Add(1) | ||
| go func( | ||
| fn func(ctx context.Context, now pcommon.Timestamp, errs chan error), | ||
| ctx context.Context, | ||
| now pcommon.Timestamp, | ||
| errs chan error) { | ||
| // actual function body | ||
| defer wg.Done() | ||
| fn(ctx, now, errs) | ||
| }(fn, ctx, now, errChan) | ||
| } | ||
|
|
||
| wg.Wait() | ||
| errChan <- nil | ||
| errs = <-errOut |
There was a problem hiding this comment.
This refactor to channels is really smooth!
greatestusername
approved these changes
Mar 19, 2024
|
@open-telemetry/collector-contrib-approvers howdy! hoping someone could take a look at this for me |
atoulme
reviewed
Mar 20, 2024
| return | ||
| // or add an error to errs | ||
| } else { | ||
| errs.Add(err) |
There was a problem hiding this comment.
// Add adds a regular error.
func (s *ScrapeErrors) Add(err error) {
s.errs = append(s.errs, err)
}
It is not threadsafe.
There was a problem hiding this comment.
How about accumulating errors in the channel and emptying the channel all at once when the scrape is done?
There was a problem hiding this comment.
sorry if im being a little dense but do i need to be worried about the threadsafety of Add() in this case? there is only one function which accesses the data in errs and it does so in a single routine.
atoulme
reviewed
Mar 20, 2024
| if err == nil || !ok { | ||
| eOut <- errs | ||
| return | ||
| // or add an error to errs |
There was a problem hiding this comment.
Suggested change
| // or add an error to errs | |
| // or add an error to errs |
dmitryax
reviewed
Apr 2, 2024
| return | ||
| case err, ok := <-eQueue: | ||
| // shutdown | ||
| if err == nil || !ok { |
There was a problem hiding this comment.
Same comment as in #31260 (comment)
dmitryax
approved these changes
Apr 4, 2024
ycombinator
pushed a commit
to ycombinator/opentelemetry-collector-contrib
that referenced
this pull request
Apr 9, 2024
…emetry#31184) **Description:** Refactored the way scrapes work to better utilize golang's language features to parallelize the scraping process. **Testing:** Existing tests cover the scrape process. Inclusion in a collector distribution was succesful.
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: Refactored the way scrapes work to better utilize golang's language features to parallelize the scraping process.
Link to tracking Issue:
Testing: Existing tests cover the scrape process. Inclusion in a collector distribution was succesful.
Documentation: No changes to documentation