[exporterhelper] enable Persistent Queue feature by default #5711

swiatekm · 2022-07-20T14:29:17Z

Description:
Persistent Queue is a feature that currently requires "enable_unstable" tag provided to the the build to enable it. We have been using it in our distro for several months already and it looks stable. It seems that others are using this capability as well (open-telemetry/opentelemetry-collector-contrib#9327 (comment))

This change enables it by default, which also simplifies the build process a little bit.

Testing:
Added some tests to cover the queue creation logic and the requeue feature. They're a bit awkward, and should probably be refactored once the requeue feature is exposed in config.

Originally posted by @pmm-sumo here, see the review discussion therein for justification for some of the choices made.

codecov · 2022-07-20T19:25:33Z

Codecov Report

Merging #5711 (7859626) into main (b463d2a) will decrease coverage by 0.38%.
The diff coverage is 94.39%.

❗ Current head 7859626 differs from pull request most recent head beacb9e. Consider uploading reports for the commit beacb9e to get more accurate results

@@            Coverage Diff             @@
##             main    #5711      +/-   ##
==========================================
- Coverage   91.78%   91.39%   -0.39%     
==========================================
  Files         192      197       +5     
  Lines       11394    11913     +519     
==========================================
+ Hits        10458    10888     +430     
- Misses        742      812      +70     
- Partials      194      213      +19

Impacted Files	Coverage Δ
...xporterhelper/internal/persistent_storage_batch.go	`80.48% <ø> (ø)`
exporter/exporterhelper/queued_retry_inmemory.go	`91.66% <93.40%> (-0.23%)`	⬇️
...porter/exporterhelper/internal/persistent_queue.go	`100.00% <100.00%> (ø)`
...rter/exporterhelper/internal/persistent_storage.go	`89.58% <100.00%> (ø)`
pdata/internal/metrics.go	`84.09% <0.00%> (-8.41%)`	⬇️
pdata/internal/common.go	`92.59% <0.00%> (-2.80%)`	⬇️
exporter/otlphttpexporter/otlp.go	`81.73% <0.00%> (-2.11%)`	⬇️
service/telemetry.go	`89.20% <0.00%> (ø)`
service/extensions/extensions.go
... and 7 more

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 b463d2a...beacb9e. Read the comment docs.

swiatekm · 2022-07-21T09:19:51Z

I've rebased this post 0.56.0 and fixed the remaining minor issues the CI got stuck on.

bogdandrutu

Would like to agree on the config first, since that is the most important part.

bogdandrutu · 2022-07-22T14:26:47Z

exporter/exporterhelper/queued_retry_inmemory.go

+	// StorageID if not empty, enables the persistent storage and uses the component specified
+	// as a storage extension for the persistent queue
+	StorageID *config.ComponentID `mapstructure:"storage"`
+	// StorageEnabled describes whether persistence via a file storage extension is enabled using the single
+	// default storage extension.
+	StorageEnabled bool `mapstructure:"persistent_storage_enabled"`


I don't like this config, is too complex for no good reason. I see no point in supporting persistent_storage_enabled instead of simply asking for the exact name.

I agree. Copying my comment from previous PR:

Is this setting necessary? If the storage is unspecified the persistent queue is disabled. Isn't that enough for control? Or do we want to have this so that we can enable the storage without knowing the name of the extension?

Alternate suggestion: can we delete persistent_storage_enabled setting and make * (or some other character sequence) a special value for storage setting which finds the only available storage extension?

The current state was an attempt to keep the config backwards-compatible, as this whole change was originally just about enabling the queue in the build by default. If we're going to break it, then I agree with @bogdandrutu - just make the user specify the storage explicitly and be done with it, looks like the best option in the long term.

If we do that though, I think the changelog should mention the change explicitly.

I would not call this a breaking change since this was an experimental feature.

It's not technically a breaking change, but it's nonetheless a feature being used in the real world, and I think it'd be polite to call it out in the changelog to make it easier for users to figure out what broke their config after an upgrade.

Either way, I'll make the change in this PR.

FYI: Waiting for the change.

Done, sorry for the delay. I've added a changelog entry to breaking changes, even though this technically isn't one.

tigrannajaryan · 2022-07-28T14:32:36Z

CHANGELOG.md

@@ -12,6 +13,7 @@

 ### 💡 Enhancements 💡

+- Enable Persistent Queue by default (#5711)


This is a bit misleading. We don't enable persistent queue by default. We just make the feature available in the default build.

tigrannajaryan · 2022-07-28T14:33:01Z

Makefile

@@ -58,7 +58,7 @@ gomoddownload:

 .PHONY: gotest
 gotest:
-	@$(MAKE) for-all-target TARGET="test test-unstable"
+	@$(MAKE) for-all-target TARGET="test"


Why is this removed? What if we have other unstable features to test?

I think we should go with feature gates for unstable features from now on, this is an old unstable feature before the gates.

tigrannajaryan · 2022-07-28T14:39:31Z

exporter/exporterhelper/queued_retry_inmemory.go

+	requeuingEnabled   bool
+	requestUnmarshaler internal.RequestUnmarshaler


Ideally this PR should do one thing: enable the feature. Instead I see modifications of the logic. Can we split the PR into 2 parts:

Change whatever logic needs to change and whatever refactoring is necessary. Keep the persistent queue feature experimental.

Make persistent queue feature part of stable build.
Otherwise I have a hard time understanding what changes are needed for enabling the feature, which changes are refactorings which don't change the behavior and what changes change the behavior.

Yeah, this became too complicated along the way. I'm going to split it into three PRs:

Make the queue logic consistent between the in-memory queue and the persistent one. This way enabling the persistent queue is just a change to the build.

Enable persistent queue in the build by default.

Configuration changes for setting the storage for the persistent queue.

The first PR: #5764. Would you prefer to close this one right now or wait until all the changes are merged individually? @tigrannajaryan @bogdandrutu ?

bogdandrutu

Please make sure we are not doing too many things in one PR, and keep PRs as small and focus as possible. For example we agreed on changing the config, let's start with doing that on the experimental feature.

bogdandrutu · 2022-07-28T18:03:10Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 ### 🛑 Breaking changes 🛑

+- Require storage explicitly set for persistent queue (#5711)


Please comment, this happens only if you enable the experimental feature xyz...

bogdandrutu · 2022-07-28T18:03:42Z

Makefile

@@ -58,7 +58,7 @@ gomoddownload:

 .PHONY: gotest
 gotest:
-	@$(MAKE) for-all-target TARGET="test test-unstable"
+	@$(MAKE) for-all-target TARGET="test"


I think we should go with feature gates for unstable features from now on, this is an old unstable feature before the gates.

bogdandrutu · 2022-07-28T18:05:37Z

exporter/exporterhelper/README.md

-[file storage extension](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/storage/filestorage).
-If collector instance is killed while having some items in the persistent queue, on restart the items are being picked and
-the exporting is continued.
+When persistent queue is enabled, the batches are being buffered to disk using [filestorage] extension. If collector instance is killed while having some items in the persistent queue, on restart the items are being picked and the exporting is continued.


wrong statement, they are buffered in the storage :) the storage can be s3 or nvram (whatever the extension implements).

bogdandrutu · 2022-07-28T18:08:53Z

exporter/exporterhelper/internal/persistent_queue.go

+// PersistentQueueStartFunc defines a function that can be used to start the persistent queue
+type PersistentQueueStartFunc func(ctx context.Context, client storage.Client)


why is this change necessary now and not before? if this can be "refactored" as a separate PR, please do so.

bogdandrutu · 2022-08-01T21:58:21Z

This needs some rework.

swiatekm · 2022-08-02T08:20:46Z

This needs some rework.

I'll just close it and continue submitting the individual changes as separate PRs. After #5764, I'll do the storage configuration change next.

swiatekm requested review from a team and tigrannajaryan July 20, 2022 14:29

tigrannajaryan mentioned this pull request Jul 20, 2022

[exporterhelper] Proposal: enable Persistent Queue feature by default #5457

Closed

swiatekm force-pushed the enable-persistent-queue-in-build branch from 7859626 to aa29c87 Compare July 21, 2022 09:16

bogdandrutu requested changes Jul 22, 2022

View reviewed changes

pmm-sumo and others added 2 commits July 28, 2022 14:43

[exporterhelper] Enable Persistent Queue feature by default

7ac484a

[exporterhelper] Require explicit storage for persistent queue

beacb9e

swiatekm force-pushed the enable-persistent-queue-in-build branch from aa29c87 to beacb9e Compare July 28, 2022 13:36

tigrannajaryan reviewed Jul 28, 2022

View reviewed changes

bogdandrutu reviewed Jul 28, 2022

View reviewed changes

aboguszewski-sumo mentioned this pull request Jul 29, 2022

[exporter/exporterhelper] allow to specify max queue size in bytes #5756

Closed

swiatekm mentioned this pull request Jul 31, 2022

[exporterhelper] Make the in-memory and persistent queues more consistent #5764

Merged

swiatekm closed this Aug 2, 2022

This was referenced Aug 2, 2022

Require the storage to be explicitly set for persistent queue #5784

Merged

Enable persistent queue in the build by default #5828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporterhelper] enable Persistent Queue feature by default #5711

[exporterhelper] enable Persistent Queue feature by default #5711

swiatekm commented Jul 20, 2022

codecov bot commented Jul 20, 2022 •

edited

Loading

swiatekm commented Jul 21, 2022

bogdandrutu left a comment

bogdandrutu Jul 22, 2022

tigrannajaryan Jul 22, 2022

swiatekm Jul 22, 2022

bogdandrutu Jul 23, 2022

swiatekm Jul 25, 2022

bogdandrutu Jul 27, 2022

swiatekm Jul 28, 2022

tigrannajaryan Jul 28, 2022

tigrannajaryan Jul 28, 2022

bogdandrutu Jul 28, 2022

tigrannajaryan Jul 28, 2022

swiatekm Jul 30, 2022

swiatekm Jul 31, 2022

bogdandrutu left a comment •

edited

Loading

bogdandrutu Jul 28, 2022

bogdandrutu Jul 28, 2022

bogdandrutu Jul 28, 2022

bogdandrutu Jul 28, 2022

bogdandrutu commented Aug 1, 2022

swiatekm commented Aug 2, 2022

		@@ -12,6 +13,7 @@

		### 💡 Enhancements 💡

		- Enable Persistent Queue by default (#5711)

		requeuingEnabled bool
		requestUnmarshaler internal.RequestUnmarshaler

		@@ -4,6 +4,7 @@

		### 🛑 Breaking changes 🛑

		- Require storage explicitly set for persistent queue (#5711)

		// PersistentQueueStartFunc defines a function that can be used to start the persistent queue
		type PersistentQueueStartFunc func(ctx context.Context, client storage.Client)

[exporterhelper] enable Persistent Queue feature by default #5711

[exporterhelper] enable Persistent Queue feature by default #5711

Conversation

swiatekm commented Jul 20, 2022

codecov bot commented Jul 20, 2022 • edited Loading

Codecov Report

swiatekm commented Jul 21, 2022

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Aug 1, 2022

swiatekm commented Aug 2, 2022

codecov bot commented Jul 20, 2022 •

edited

Loading

bogdandrutu left a comment •

edited

Loading