[chore] Do not introduce pointers in the deprecated batcher fields #12496
Conversation
dmitryax
force-pushed
the
avoid-int-pointer-changes
branch
from
f3d6477 to
b205218
Compare
dmitryax
force-pushed
the
avoid-int-pointer-changes
branch
from
b205218 to
ec3b179
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (80.00%) 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 #12496 +/- ##
==========================================
+ Coverage 92.15% 92.18% +0.02%
==========================================
Files 465 465
Lines 25183 25183
==========================================
+ Hits 23208 23214 +6
+ Misses 1576 1572 -4
+ Partials 399 397 -2 ☔ View full report in Codecov by Sentry. |
| @@ -70,7 +70,7 @@ func NewBaseExporter(set exporter.Settings, signal pipeline.Signal, options ...O | |||
| } | |||
|
|
|||
| //nolint: staticcheck | |||
| if be.batcherCfg.MinSizeItems != nil || be.batcherCfg.MaxSizeItems != nil { | |||
| if be.batcherCfg.MinSizeItems != 0 || be.batcherCfg.MaxSizeItems != 0 { | |||
There was a problem hiding this comment.
Potentially, we could update the Unmarshal func to set a private field indicating whether min/max_size_items were set, but that would require moving/aliasing the exporterbatcher.Config in some internal module along with a function showing this warning. I could do that separately.
Do not introduce an unnecessary Go API breaking change
dmitryax
force-pushed
the
avoid-int-pointer-changes
branch
from
ec3b179 to
166e662
Compare
| if c.MinSizeItems != nil { | ||
| c.SizeConfig.MinSize = *c.MinSizeItems | ||
| if conf.IsSet("min_size_items") { | ||
| c.SizeConfig.MinSize = c.MinSizeItems |
There was a problem hiding this comment.
Not directly related to the PR, but the deprecation comment says that min_size_items is "ignored if SizeConfig is set". Doesn't this code do the opposite, ie. ignore SizeConfig if min/max_size_items are set?
There was a problem hiding this comment.
Good catch. I can submit another PR to address this
|
Some contrib tests still needs to be updated. So maybe it's ok to switch to pointers to keep it simple. I'll proceed with open-telemetry/opentelemetry-collector-contrib#38214 |
Do not introduce an unnecessary Go API breaking change.
Fixes contrib tests.
A follow-up to #12486.