[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 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.13%. Comparing base (2e9c827) to head (bef7f11).
⚠️ Report is 1 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 (85.71%) 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.15%   92.13%   -0.02%     
==========================================
  Files         666      666              
  Lines       41441    41436       -5     
==========================================
- Hits        38191    38179      -12     
- Misses       2213     2217       +4     
- Partials     1037     1040       +3     

☔ 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.

[jmacd]

I will take over this PR. We will remove the configioptional.Optional[] wrapper on the default queue settings object.

[codspeed-hq]

CodSpeed Performance Report

Merging #14155 will improve performances by ×280

_{Comparing mx-psi:mx-psi/configoptional-for-exporterhelper-queuebatchconfig (bef7f11) with main (10f119c)¹}

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

⚡ 1 improvement
✅ 114 untouched
⏩ 1 skipped²

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	BenchmarkPersistentQueue	206,515 ns	741 ns	×280

Footnotes

1. No successful run was found on main (1578cf8) during the generation of this report, so 10f119c was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

2. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩

[codspeed-hq]

Unable to generate the flame graphs

The performance report has correctly been generated, but there was an internal error while generating the flame graphs for this run. We're working on fixing the issue. Feel free to contact us on Discord or at [email protected] if the issue persists.

[evan-bradley]

Changes look good to me. 👍

[mx-psi]

I can't formally approve since I am the author but the changes from @jmacd look good to me

[mx-psi]

@jmacd Let me know when you are ready for me to merge the PR!