[exporterhelper] Use configoptional.Optional for exporterhelper QueueBatchConfig

[mx-psi]

Description

Uses configoptional.Optional for exporterhelper.QueueBatchConfig.

Link to tracking issue

Updates #14021

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.14%. Comparing base (8da559f) to head (b5a6743).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
exporter/exporterhelper/internal/base_exporter.go	85.71%	0 Missing and 1 partial ⚠️
...rter/exporterhelper/xexporterhelper/new_request.go	0.00%	1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (84.61%) 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   #14155      +/-   ##
==========================================
- Coverage   92.25%   92.14%   -0.12%     
==========================================
  Files         658      663       +5     
  Lines       41186    41384     +198     
==========================================
+ Hits        37995    38132     +137     
- Misses       2184     2214      +30     
- Partials     1007     1038      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[songy23]

Do you want to merge this PR first or fix contrib first?

[mx-psi]

I'll prepare a contrib PR before merging this

[jmacd]

This is great! The new configoptional is such an improvement.

I officially volunteer to help fix the contrib repo.

[jmacd]

Maintainers: https://cloud-native.slack.com/archives/C02JJK840SH/p1763412058572739

We've been discussing open-telemetry/opentelemetry-collector-contrib#44320, where I've proposed that a helper method like

// Disabled converts the value to a Default, keeping the settings but
// disabling the option. This ensures that when a user enables it, the
// default settings remain.
func (o Optional[T]) Disabled() (result Optional[T]) {
	result = o
	result.flavor = defaultFlavor
	return
}

would help ergonomics and test readability. That doesn't entirely solve the problem, we're seeing tests that fail because of the Optional.flavor field, which may be "Default" for the test expetation and "None" from the Yaml parser, or something similar.

[evan-bradley]

Following up from an offline discussion, would it be a major inconvenience if we just returned the config directly here and required consumers to wrap it themselves? I feel like it would make tests easier and would make the function slightly less opinionated.

[jmacd]

I agree!

I still feel something's not perfect. We're recommending that users call configoptional.Default(...) or configoptional.Some(...) in order to disable or enable a feature by default. It sort of suggests adding a function named configoptional.Enabled(...) (for enabled with defaults) and configoptional.Disabled(...) (for disabled with defaults).

[evan-bradley]

I see your concern. I think this partially sources from the fact that we have the enabled field in configoptional now, as before it was just a way to think about the presence of a value at the time the function is called. The best I can think of off the top of my head is aliasing each function to EnabledByDefault and DisabledByDefault or similar, but I'd want to give it more thought before committing. If we go this route, would want the names to be very explicit since I think Enabled and Disabled are also a little ambiguous.

[jmacd]

For QueueBatchConfig, there are many components that have the feature disabled by default. I'm worried that many users if they're just copying from another component, might find the use of configoptional.None[...] and think of it as a good way to disable a feature by default.

But then, they're throwing away all the default settings, and users who want to enable this feature will have to supply all the fields with valid values, and new fields could break existing configs.

I think we want this "disabled with defaults" case in every one of the places where cfg.Feature.Disabled = false appears today, and the way to spell this is configoptional.Default(default). I'm concerned that this call doesn't say what it means.

I guess I'm asking if we can have a configoptional function called Disabled(default) or DisabledWithDefault(default) which does what it says, rather than requiring callers to understand the three-states of an Optional.

[evan-bradley]

@jmacd I see your point. We could probably distill our Some, Default, and None options into:

• Some: EnabledWithDefault(Config{...})
• Default: DisabledWithDefault(Config{...})
• None: DisabledWithDefault(Config{})

I may be missing a case or two here, but this seems viable at a glance.

It would be unfortunate since we just declared this module v1, but I think we could easily declare each function deprecated with an easy replacement path.

[evan-bradley]

However, I think the behavior is somewhat complex since the functionality is somewhat complex, so we may just need to solve this with documentation either way.