-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: breaking | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) | ||
component: exporterhelper | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Make the re-enqueue behavior configurable. | ||
|
||
# One or more tracking issues or pull requests related to the change | ||
issues: [8987] | ||
|
||
subtext: | | ||
Instead of relying on the 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 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. | ||
|
||
IMPORTANT: Make sure to set `sending_queue::requeue_on_failure` to `true` if you use persistent queue with | ||
`retry_on_failure` enabled to preserve the same behavior. | ||
|
||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [user] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,14 @@ type QueueSettings struct { | |
// StorageID if not empty, enables the persistent storage and uses the component specified | ||
// as a storage extension for the persistent queue | ||
StorageID *component.ID `mapstructure:"storage"` | ||
// RequeueOnFailure indicates whether requests are requeued or dropped on non-permanent failures. | ||
// Do not confuse this option with the retry_on_failure which consumes | ||
// requests from the queue and apply blocking exponential backoff retry mechanism. | ||
// This option is applied after all the retries configured with retry_on_failure are exhausted. | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. Any reasons we want
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. It's not even always applicable to the persistent queue. Only if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
} | ||
|
||
// NewDefaultQueueSettings returns the default settings for QueueSettings. | ||
|
@@ -89,22 +97,20 @@ type queueSender struct { | |
func newQueueSender(config QueueSettings, set exporter.CreateSettings, signal component.DataType, | ||
marshaler RequestMarshaler, unmarshaler RequestUnmarshaler) *queueSender { | ||
|
||
isPersistent := config.StorageID != nil | ||
var queue internal.Queue[Request] | ||
if isPersistent { | ||
if config.StorageID != nil { | ||
queue = internal.NewPersistentQueue[Request]( | ||
config.QueueSize, signal, *config.StorageID, marshaler, unmarshaler, set) | ||
} else { | ||
queue = internal.NewBoundedMemoryQueue[Request](config.QueueSize) | ||
} | ||
qs := &queueSender{ | ||
fullName: set.ID.String(), | ||
queue: queue, | ||
traceAttribute: attribute.String(obsmetrics.ExporterKey, set.ID.String()), | ||
logger: set.TelemetrySettings.Logger, | ||
meter: set.TelemetrySettings.MeterProvider.Meter(scopeName), | ||
// TODO: this can be further exposed as a config param rather than relying on a type of queue | ||
requeuingEnabled: isPersistent, | ||
fullName: set.ID.String(), | ||
queue: queue, | ||
traceAttribute: attribute.String(obsmetrics.ExporterKey, set.ID.String()), | ||
logger: set.TelemetrySettings.Logger, | ||
meter: set.TelemetrySettings.MeterProvider.Meter(scopeName), | ||
requeuingEnabled: config.RequeueOnFailure, | ||
} | ||
qs.consumers = internal.NewQueueConsumers(queue, config.NumConsumers, qs.consume) | ||
return qs | ||
|
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 haveretry_on_failure
, so we probably should stay consistent