-
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
Enable persistent queue in the build by default #5828
Enable persistent queue in the build by default #5828
Conversation
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
3b0a261
to
4ff829b
Compare
Probably a good idea |
4ff829b
to
46dca85
Compare
You should delete the experimental not the inmemory |
Why not just copy the code to |
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. |
46dca85
to
321eb56
Compare
321eb56
to
267e107
Compare
Ok, I see what you mean after having done it. |
// 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"` |
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.
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.
"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 |
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.
Please file an issue to followup and merge this with queue_retry (we should do it in a separate PR).
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 |
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.
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.
@@ -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 |
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.
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.
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.
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>
Co-authored-by: Bogdan Drutu <lazy@splunk.com>
|
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.
LGTM
@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 |
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
toqueued_retry_inmemory.go
to make it easier to see the changes. This can be further cleaned up - the contents ofqueued_retry_inmemory.go
should go intoqueued_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.