-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Remove old push based batcher #12233
Remove old push based batcher #12233
Conversation
"go.opentelemetry.io/collector/pipeline" | ||
) | ||
|
||
var usePullingBasedExporterQueueBatcher = featuregate.GlobalRegistry().MustRegister( | ||
"exporter.UsePullingBasedExporterQueueBatcher", | ||
featuregate.StageBeta, |
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.
In the previous release was alpha, turned to beta in this release. Decided to remove this sooner, since we want to complete the exportehelper this cycle.
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 Deprecated, not Stable?
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.
Because it was alpha:
Features that prove unworkable in the alpha stage may be discontinued without proceeding to the beta stage. Instead, they will proceed to the deprecated stage, which will feature is permanently disabled. A feature gate will be removed once it has been deprecated for at least 2 releases of the collector.
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.
But this feature hasn't proved unworkable. We still want to proceed with this, right?
f048ea8
to
047b8ea
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (83.33%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12233 +/- ##
==========================================
- Coverage 91.30% 91.13% -0.17%
==========================================
Files 464 462 -2
Lines 25662 25483 -179
==========================================
- Hits 23430 23224 -206
- Misses 1819 1848 +29
+ Partials 413 411 -2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Bogdan Drutu <[email protected]>
047b8ea
to
77deac9
Compare
Do we have a replacement for batching-only behavior? I don't think we initialize a 0-sized blocking queue if the queue is disabled and only batching is enabled... At least the queue code path isn't executed as far as I can tell. |
"go.opentelemetry.io/collector/pipeline" | ||
) | ||
|
||
var usePullingBasedExporterQueueBatcher = featuregate.GlobalRegistry().MustRegister( | ||
"exporter.UsePullingBasedExporterQueueBatcher", | ||
featuregate.StageBeta, |
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 Deprecated, not Stable?
@@ -101,23 +100,8 @@ func NewBaseExporter(set exporter.Settings, signal pipeline.Signal, osf ObsrepSe | |||
be.QueueSender = NewQueueSender(q, be.Set, be.queueCfg.NumConsumers, be.ExportFailureMessage, be.Obsrep, be.BatcherCfg) | |||
} | |||
|
|||
if !usePullingBasedExporterQueueBatcher.IsEnabled() && be.BatcherCfg.Enabled || | |||
usePullingBasedExporterQueueBatcher.IsEnabled() && be.BatcherCfg.Enabled && !be.queueCfg.Enabled { | |||
bs := NewBatchSender(be.BatcherCfg, be.Set) |
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.
Even with the feature gate set to Stable, we still have this condition be.BatcherCfg.Enabled && !be.queueCfg.Enabled
because we haven't replaced the batching-only use case
Replaced by #12425 |
See https://github.com/open-telemetry/opentelemetry-collector/tree/main/featuregate#feature-lifecycle