Enable persistent queue in the build by default #5828

swiatekm · 2022-08-05T13:41:22Z

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.

This PR simply moves queued_retry_experimental.go to queued_retry_inmemory.go to make it easier to see the changes. This can be further cleaned up - the contents of queued_retry_inmemory.go should go into queued_retry.go, and the same with their respective tests. I'm planning to submit a separate PR with the cleanup to make this one easier to review.

This was originally part of #5711, but was then split into multiple PRs for clarity. #5764 and #5784 were already merged, and this is the final change.

Link to tracking Issue: N/A

Documentation: Changed the queue status from experimental to alpha.

codecov · 2022-08-05T13:47:17Z

Codecov Report

Merging #5828 (c320f9b) into main (ba6fe5a) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5828      +/-   ##
==========================================
- Coverage   91.79%   91.74%   -0.05%     
==========================================
  Files         191      196       +5     
  Lines       11478    11948     +470     
==========================================
+ Hits        10536    10962     +426     
- Misses        751      778      +27     
- Partials      191      208      +17

Impacted Files	Coverage Δ
...porter/exporterhelper/internal/persistent_queue.go	`100.00% <ø> (ø)`
...rter/exporterhelper/internal/persistent_storage.go	`89.49% <ø> (ø)`
...xporterhelper/internal/persistent_storage_batch.go	`80.48% <ø> (ø)`
exporter/exporterhelper/queued_retry_inmemory.go	`95.58% <100.00%> (+16.24%)`	⬆️
extension/experimental/storage/nop_client.go	`33.33% <0.00%> (ø)`
extension/experimental/storage/storage.go	`100.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

TylerHelmuth · 2022-08-05T14:46:40Z

If the amount of changes makes this hard to understand, I can just submit the build changes and deletion of queued_retry_inmemory.go, and do the rest in a separate PR.

Probably a good idea

bogdandrutu · 2022-08-05T15:17:53Z

You should delete the experimental not the inmemory

swiatekm · 2022-08-05T15:59:53Z

You should delete the experimental not the inmemory

Why not just copy the code to queued_retry.go in that case? As far as I know, the only reason it's not there to begin with is the in-memory/experimental split. I wanted to do that as a follow-up PR to this one, but I'm good either way.

bogdandrutu · 2022-08-05T16:36:51Z

Why not just copy the code to queued_retry.go in that case? As far as I know, the only reason it's not there to begin with is the in-memory/experimental split. I wanted to do that as a follow-up PR to this one, but I'm good either way.

Because this way simplifies the reviewer job, and can easily see what changes, everything that is added in the inmemory which is right now used.

swiatekm · 2022-08-05T16:50:07Z

Why not just copy the code to queued_retry.go in that case? As far as I know, the only reason it's not there to begin with is the in-memory/experimental split. I wanted to do that as a follow-up PR to this one, but I'm good either way.

Because this way simplifies the reviewer job, and can easily see what changes, everything that is added in the inmemory which is right now used.

Ok, I see what you mean after having done it.

exporter/exporterhelper/queued_retry_inmemory.go

bogdandrutu · 2022-08-05T16:57:02Z

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"`


Please file an issue to followup and extract this into a configstorage similar with configauth. We can discuss in the issue what we need to do.

bogdandrutu · 2022-08-05T16:57:38Z

exporter/exporterhelper/queued_retry_inmemory.go

 	"go.opentelemetry.io/collector/internal/obsreportconfig/obsmetrics"
 )

-// queued_retry_inmemory includes the code for memory-backed (original) queued retry helper only
-// enabled when "enable_unstable" build tag is not set
+// queued_retry_inmemory includes the code for both memory-backed and persistent-storage backed queued retry helpers


Please file an issue to followup and merge this with queue_retry (we should do it in a separate PR).

bogdandrutu · 2022-08-05T16:58:54Z

exporter/exporterhelper/queued_retry_inmemory.go

+	if qCfg.StorageID == nil {
+		qrs.queue = internal.NewBoundedMemoryQueue(qrs.cfg.QueueSize, func(item interface{}) {})
+	}
+	// The Persistent Queue is initialized separately as it needs extra information about the component


Why initializing the in memory here versus doing in the same place with the persistent and just have a initializeQueue. To not do too many changes in this PR, probably best to create an issue and do a followup PR.

bogdandrutu · 2022-08-05T16:59:51Z

exporter/exporterhelper/queued_retry_experimental_test.go

@@ -12,9 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-//go:build enable_unstable
-// +build enable_unstable
-
 package exporterhelper


Followup to rename this file or merge into the queue_retry_inmemory_test. We should file an issue to not forget but a different PR is better.

I'm going to submit a PR cleaning this up after this one is merged, don't think we need an issue for it.

Co-authored-by: Bogdan Drutu <lazy@splunk.com>

bogdandrutu · 2022-08-05T17:21:01Z

make[2]: Leaving directory '/home/runner/work/opentelemetry-collector/opentelemetry-collector/cmd/otelcorecol'
level=warning msg="[linters context] structcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
exporter/exporterhelper/queued_retry_inmemory.go:156: File is not `gofmt`-ed with `-s` (gofmt)

TylerHelmuth

LGTM

TylerHelmuth · 2022-08-05T17:38:13Z

@swiatekm-sumo if you need it I can help out with some of the new issues that were asked to be created.

swiatekm · 2022-08-05T17:41:08Z

@swiatekm-sumo if you need it I can help out with some of the new issues that were asked to be created.

I think I'm going to be fine. I'll create the configstorage issue, and the rest will just be relatively simple PRs.

swiatekm force-pushed the persistent-queue-by-default branch 2 times, most recently from 3b0a261 to 4ff829b Compare August 5, 2022 14:45

swiatekm marked this pull request as ready for review August 5, 2022 14:45

swiatekm requested review from a team and jpkrohling August 5, 2022 14:45

swiatekm force-pushed the persistent-queue-by-default branch from 4ff829b to 46dca85 Compare August 5, 2022 15:00

swiatekm force-pushed the persistent-queue-by-default branch from 46dca85 to 321eb56 Compare August 5, 2022 16:47

Enable persistent queue in the build by default

267e107

swiatekm force-pushed the persistent-queue-by-default branch from 321eb56 to 267e107 Compare August 5, 2022 16:49

bogdandrutu reviewed Aug 5, 2022

View reviewed changes

Mikołaj Świątek and others added 2 commits August 5, 2022 19:07

Update exporter/exporterhelper/queued_retry_inmemory.go

9acfc1e

Co-authored-by: Bogdan Drutu <lazy@splunk.com>

Update exporter/exporterhelper/queued_retry_inmemory.go

1556698

Co-authored-by: Bogdan Drutu <lazy@splunk.com>

bogdandrutu approved these changes Aug 5, 2022

View reviewed changes

Fix formatting

c320f9b

TylerHelmuth approved these changes Aug 5, 2022

View reviewed changes

bogdandrutu merged commit 5f3b0d8 into open-telemetry:main Aug 5, 2022

swiatekm deleted the persistent-queue-by-default branch August 5, 2022 17:53

swiatekm mentioned this pull request Aug 5, 2022

Clean up queued retry code after persistent queue merge #5835

Merged

pmm-sumo mentioned this pull request Aug 11, 2022

Persistent Queue issues/questions #5894

Open

mx-psi mentioned this pull request Sep 14, 2022

[docs] Update release instructions #6068

Merged

mx-psi mentioned this pull request Sep 28, 2022

[receiver/otlp] Rewrite logs related to server startup #6174

Merged

mx-psi mentioned this pull request Sep 29, 2022

[pdata] Update deprecation comments #6206

Merged

swiatekm mentioned this pull request May 26, 2023

REQUEST: New membership for @swiatekm-sumo open-telemetry/community#1512

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable persistent queue in the build by default #5828

Enable persistent queue in the build by default #5828

swiatekm commented Aug 5, 2022 •

edited

Loading

codecov bot commented Aug 5, 2022 •

edited

Loading

TylerHelmuth commented Aug 5, 2022

bogdandrutu commented Aug 5, 2022

swiatekm commented Aug 5, 2022

bogdandrutu commented Aug 5, 2022

swiatekm commented Aug 5, 2022

bogdandrutu Aug 5, 2022

bogdandrutu Aug 5, 2022

bogdandrutu Aug 5, 2022

bogdandrutu Aug 5, 2022

swiatekm Aug 5, 2022 •

edited

Loading

bogdandrutu commented Aug 5, 2022

TylerHelmuth left a comment

TylerHelmuth commented Aug 5, 2022

swiatekm commented Aug 5, 2022

Enable persistent queue in the build by default #5828

Enable persistent queue in the build by default #5828

Conversation

swiatekm commented Aug 5, 2022 • edited Loading

codecov bot commented Aug 5, 2022 • edited Loading

Codecov Report

TylerHelmuth commented Aug 5, 2022

bogdandrutu commented Aug 5, 2022

swiatekm commented Aug 5, 2022

bogdandrutu commented Aug 5, 2022

swiatekm commented Aug 5, 2022

bogdandrutu Aug 5, 2022

Choose a reason for hiding this comment

bogdandrutu Aug 5, 2022

Choose a reason for hiding this comment

bogdandrutu Aug 5, 2022

Choose a reason for hiding this comment

bogdandrutu Aug 5, 2022

Choose a reason for hiding this comment

swiatekm Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

bogdandrutu commented Aug 5, 2022

TylerHelmuth left a comment

Choose a reason for hiding this comment

TylerHelmuth commented Aug 5, 2022

swiatekm commented Aug 5, 2022

swiatekm commented Aug 5, 2022 •

edited

Loading

codecov bot commented Aug 5, 2022 •

edited

Loading

swiatekm Aug 5, 2022 •

edited

Loading