[exporterhelper] Add queue options to the new exporter helper #8248
Conversation
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
dmitryax
force-pushed
the
exporterhelper-v2-enable-queue-retry
branch
2 times, most recently
from
0589798
to
bf50039
Compare
dmitryax
force-pushed
the
exporterhelper-v2-enable-queue-retry
branch
2 times, most recently
from
cd900e4
to
b8af2fb
Compare
exporter/exporterhelper/common.go
Outdated
| // WithMemoryQueue overrides the default QueueConfig for an exporter to use an in-memory queue. | ||
| func WithMemoryQueue(config QueueConfig) Option { | ||
| return func(o *baseSettings) { | ||
| o.queueSettings.config = QueueSettings{ | ||
| Enabled: config.Enabled, | ||
| NumConsumers: config.NumConsumers, | ||
| QueueSize: config.QueueSize, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // WithPersistentQueue overrides the default QueueConfig for an exporter to use a persistent queue. | ||
| // This option can be used only with the new exporter helpers New[Traces|Metrics|Logs]RequestExporter. | ||
| func WithPersistentQueue(config PersistentQueueConfig, marshaler RequestMarshaler, unmarshaler RequestUnmarshaler) Option { |
There was a problem hiding this comment.
To avoid possible miss-usage of these APIs by having users configuring both queues, should we have an API that avoids that? Something like this (not necessary this way, but the idea like this):
func WithQueue(config *QueueConfig) Option
type QueueConfig struct {
...
}
func NewMemoryQueueConfig(...) QueueConfig
func NewPersistentQueueConfig(...) QueueConfig
There was a problem hiding this comment.
Or func WithQueue(config *QueueOption) Option
There was a problem hiding this comment.
Alternative for even more generic, you have func WithQueue(queue Queue) Option, where Queue is the current ProducerConsumerQueue. This way we can have the queue implementation outside of this package.
There was a problem hiding this comment.
I like func WithQueue(queue Queue) Option. It makes future improvements/additions to the queues easier. But it requires some refactoring because the persistent queue is initialized in two steps currently. I'm trying this out. It has to be refactored anyway I believe
There was a problem hiding this comment.
@bogdandrutu I submitted a draft PR with the refactoring allowing the WithQueue option #8275. PTAL once you have a chance
There was a problem hiding this comment.
@bogdandrutu, following the discussion in #8284 (comment), I'm now more inclined towards this PR other than #8275 due to the following reasons:
-
Exposing
Queueinterface with theStartmethod is fine, butCallbackand other arguments outlined in [chore] [exporterhelper] Refactor queue initialization #8284 (comment) cannot be part of the constructors exposed to the users. So, if we don't want them passed asStartarguments, we need to encapsulate the internal Queue and hide these arguments. Probably it'll look more like another option that you suggesdtedfunc WithQueue(config *QueueOption) Option -
Using the interface from this PR seems like requires the least amount of code to write for the user.
Do you think WithQueue is also better in case we might want to introduce another type of Queue in future?
There was a problem hiding this comment.
The big downside for this is that this package must depend on the queue implementations so if we need to extend in the future we need to grow this package and its dependencies (if extension needs for example S3 queue). Maybe directly Queue is hard to expose, but we can have WithQueue(factory QueueFactory) where the factory gets the Callback as part of the initialization settings.
There was a problem hiding this comment.
@bogdandrutu I see your point. I took your recommendations from this thread and #8435 into account and implemented #8275. I had to introduce a few more packages to avoid circular dependencies. PTAL and let me know if that is close to how you envision it.
dmitryax
force-pushed
the
exporterhelper-v2-enable-queue-retry
branch
9 times, most recently
from
415aa5a
to
8070a59
Compare
dmitryax
force-pushed
the
exporterhelper-v2-enable-queue-retry
branch
2 times, most recently
from
77291e4
to
cb206be
Compare
This change enabled queue capability for the new exporter helper. For now, it preserves the same user configuration interface as the existing exporter helper has. The only difference is that implementing persistence is optional now as it requires providing marshal and unmarshal functions for the custom request. Later, it's possible to introduce more options for controlling the queue: count of items or bytes in the queue.
dmitryax
force-pushed
the
exporterhelper-v2-enable-queue-retry
branch
from
cb206be
to
cf07b8f
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Introduce a way to enable queue in the new exporter helper with a developer interface suggested in #8248 (comment). The new configuration interface for the end users provides a new `queue_size_items` option to limit the queue by a number of spans, log records, or metric data points. The previous way to limit the queue by number of requests is preserved under the same field, `queue_size,` which will later be deprecated through a longer transition process. Tracking issue: #8122
This change enabled queue capability for the new exporter helper. For now, it preserves the same user configuration interface as the existing exporter helper has. The only difference is that implementing persistence is optional now as it requires providing marshal and unmarshal functions for the custom request.
Later, it's possible to introduce more options for controlling the queue: count of items or bytes in the queue.
Tracking issue: #8122