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

Allow partial retries in queued retry processor #1297

Merged

Conversation

pavolloffay
Copy link
Member

Signed-off-by: Pavol Loffay ploffay@redhat.com

Description: <Describe what has changed.

This patch allows partial retries in queued_retry processor. The following consumer after the retry processor initializes partial retry by returning an error type that encapsulates data that should be retried. When a different error type is returned the processor retries the original data set.

This PR does not contain any breaking changes nor change of the current behavior of the queued_retry processor.

Link to tracking Issue:

Resolves #990

Testing: < Describe what testing was performed and which tests were added.>

Documentation: < Describe the documentation added.>

Not needed only godoc.

@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #1297 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1297      +/-   ##
==========================================
+ Coverage   89.42%   89.49%   +0.06%     
==========================================
  Files         212      213       +1     
  Lines       15170    15179       +9     
==========================================
+ Hits        13566    13584      +18     
+ Misses       1170     1163       -7     
+ Partials      434      432       -2     
Impacted Files Coverage Δ
consumer/consumererror/permanenterror.go 77.77% <ø> (ø)
consumer/consumererror/partialerror.go 100.00% <100.00%> (ø)
processor/queuedprocessor/queued_processor.go 79.23% <100.00%> (+2.85%) ⬆️
service/builder/exporters_builder.go 74.28% <0.00%> (+1.42%) ⬆️
exporter/opencensusexporter/opencensus.go 49.45% <0.00%> (+4.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ab219...22d4e11. Read the comment docs.

@owais
Copy link
Contributor

owais commented Jul 8, 2020

Does this mean exporters need to know about the existence of queued processor and may be use it as a dependency (to import the error type)? Can we may be move the error type to the components or some other core package and generalize it so it's not highly specific to queued retry? We could rename it to something generic and also rename the toRetry field to something like failures or failed. We'd also need different error types for metrics, traces and logs or may be we can have failedMetrics, failedTraces fields on the same error? Once we have this, exporters can freely chose to use return this error and any component in the pipeline before can add support to handle partial failures. This would enable people to implement custom error handling/retrying processors that can also support partial retries.

@pavolloffay
Copy link
Member Author

Does this mean exporters need to know about the existence of queued processor and may be use it as a dependency

correct, currently exporters would have to import it if they want to use the error type.

We'd also need different error types for metrics, traces and logs or may be we can have failedMetrics, failedTraces fields on the same error?

A different error type would work, or a single type with multiple factory methods and getters for data types.

@pavolloffay
Copy link
Member Author

I have moved the error to the consumer package and made it more generic.

@bogdandrutu
Copy link
Member

Please rebase I made some refactoring in the qretry

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay
Copy link
Member Author

Rebased

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@@ -174,6 +174,9 @@ func (sp *queuedSpanProcessor) processItemFromQueue(item *queueItem) {
// throw away the batch
sp.onItemDropped(item, err)
return
} else if partialErr, isPartial := err.(consumererror.PartialError); isPartial {
Copy link
Member

Choose a reason for hiding this comment

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

Don't need else if because the previous returns. just an if

Copy link
Member

Choose a reason for hiding this comment

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

Will fix this in my metrics PR.

@bogdandrutu bogdandrutu merged commit d8c017a into open-telemetry:master Jul 10, 2020
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
* Allow partial retries in queued retry processor

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Rename consumererror file

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Fix data race

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
…ry#1297)

Bumps [github.com/signalfx/golib/v3](https://github.com/signalfx/golib) from 3.3.43 to 3.3.44.
- [Release notes](https://github.com/signalfx/golib/releases)
- [Commits](signalfx/golib@v3.3.43...v3.3.44)

---
updated-dependencies:
- dependency-name: github.com/signalfx/golib/v3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

Allow partial retries of trace data in queued retry processor
3 participants