-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[exporterhelper] Make the re-enqueue behavior configurable #8993
Conversation
5fe217b
to
7ab6933
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8993 +/- ##
==========================================
- Coverage 91.57% 91.56% -0.01%
==========================================
Files 316 316
Lines 17147 17138 -9
==========================================
- Hits 15702 15693 -9
Misses 1150 1150
Partials 295 295 ☔ View full report in Codecov by Sentry. |
7ab6933
to
a7685b0
Compare
Needs a rebase. Also I am not convinced that this is clear to the users, I am worried that will get them more confused. |
a7685b0
to
56096c6
Compare
@bogdandrutu I rebased and updated the config option description. PTAL if it's more clear now |
56096c6
to
4658926
Compare
// If true, a request failed with a non-permanent error will be put back in the queue and | ||
// retried after the current queue is drained. | ||
// If false (default), all failed requests will be dropped. | ||
RequeueOnFailure bool `mapstructure:"requeue_on_failure"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequeueOnTransientFailure
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requeue_on_transient_failure
seems like a lot of typing. Also we have retry_on_failure
, so we probably should stay consistent
4658926
to
526cdf6
Compare
526cdf6
to
de80498
Compare
Instead of relying or enabled `retry_on_failure` option, we now have a new option `sending_queue::requeue_on_failure` to control the requeuing independently of the retry sender. This can be useful for users who don't want the blocking exponential retry, just want to put the failed request back to the queue. This option can also be enabled with memory queue now, which means that the data will never be dropped after getting to the queue as long as the collector is up and running.
de80498
to
4b815ff
Compare
// If true, a request failed with a non-permanent error will be put back in the queue and | ||
// retried after the current queue is drained. | ||
// If false (default), all failed requests will be dropped. | ||
RequeueOnFailure bool `mapstructure:"requeue_on_failure"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reasons we want requeue_on_failure
rather than drop_data_on_failure
?
- By default, users don't like data loss. If they miss out on this configuration they would have data loss unexpectedly.
- Having
drop_data_on_failure
with default to false would make this change backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's the case for the persistent queue only. Memory queue has the opposite behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not even always applicable to the persistent queue. Only if retry_on_failure::enabled=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably do some transition period when this option is preset if persistent queue + retry_on_failure are enabled with a warning asking users to set this explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm i see. so this is also aligning the behavior between memory/persistent queue. yeah having some WARN logs would probably be helpful in this transition time.
#9054 should be merged first, otherwise, requeue to the memory queue on shutdown potentially can panic |
We decided to remove requeuing instead #9090 |
Introduce a new option
sending_queue::requeue_on_failure
to control the requeuing independently of the retry sender. This can be useful for users who don't want the blocking exponential retry and just want to put the failed request back in the queue. This option can also be enabled with the memory queue now, which means that the data will never be dropped after getting to the queue as long as the collector is up and running.Resolves #8987