[configopaque] Mark as stable

[mx-psi]

Description: Fixes #9167

[bogdandrutu]

Feedback that I would like to make sure we address:

• Should we apply the same reasoning as @mx-psi did for component.Type and make this a struct that has a for example a string Raw() to access the opaque string?
• https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configopaque/opaque_test.go uses yaml/v3 but we don't use that to unmarshal our configs. Should we actually test with confmap?
• Not necessary impacting this, but would like all the implementation checks to be moved to tests (see https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configopaque/opaque.go#L17) so that the linker does unnecessary bring that dependency to non-test code if unnecessary. In this case is a system func so not that worry, but want to set a pattern.

[mx-psi]

Should we apply the same reasoning as @mx-psi did for component.Type and make this a struct that has a for example a string Raw() to access the opaque string?

Not all strings are valid component.Types, but all strings are valid configopaque.Strings, so I don't think we get anything out of making this a struct.

[bogdandrutu]

Not all strings are valid component.Types, but all strings are valid configopaque.Strings, so I don't think we get anything out of making this a struct.

Indeed, but I am worried about usability, I feel that explicitly have to call a func to get access to the underlying string clarifies the usage intent. This is not a blocker, but want to make sure we agree that is not an improvement and doesn't worth the effort.

[atoulme]

• Not necessary impacting this, but would like all the implementation checks to be moved to tests (see https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configopaque/opaque.go#L17) so that the linker does unnecessary bring that dependency to non-test code if unnecessary. In this case is a system func so not that worry, but want to set a pattern.

https://github.com/open-telemetry/opentelemetry-collector/pull/9430/files is open to follow up on this.

[TylerHelmuth]

Indeed, but I am worried about usability, I feel that explicitly have to call a func to get access to the underlying string clarifies the usage intent.

I agree that something like

s := configopaque.String("api-key")
r.Headers.Set("api-key", s.Raw())

is clearer than

s := configopaque.String("api-key")
r.Headers.Set("api-key", string(s))

We could update the expectation to be:

// The only way to view the value stored in a configopaque.String is to call `Raw`.

A struct may also be more future proof way to ensure the underlying value isn't exposed like it used to be before we implemented Stringer and the other Marshalers.

But also, it be a big breaking change - at this point the use is quite widespread - the benefits may not outweigh the costs.

[mx-psi]

Summary table of pros/cons (happy to edit and add more):

	configopaque.String is a string alias	configopaque.String is a struct
Pros	- You can assign a string literal to configopaque.String fields - No need for breaking changes	- More explicit way of getting the value of a configopaque.String
Cons	- Less explicit way of getting the value out of a configopaque.String (type cast)	- You need to explicitly use the constructor to assign to a configopaque.String field - We need to break everybody :(

[mx-psi]

Should we actually test with confmap?

The way I see it we should test this on confmap:

1. confmap should work correctly with Go types that conform to standard Go interfaces
2. configopaque implements all the standard Go interfaces
3. Therefore --> confmap should behave nicely with configopaque, and it's on confmap to test this

[bogdandrutu]

@mx-psi then why do we test it with yaml/v3? What is special about it?

[bogdandrutu]

But also, it be a big breaking change - at this point the use is quite widespread - the benefits may not outweigh the costs.

We can do it to not break contrib that bad:

• Add constructor; Move everyone to this
• Add getter func; Move everyone to this

Then change.

[mx-psi]

@mx-psi then why do we test it with yaml/v3? What is special about it?

Nothing special, just something that uses encoding.TextMarshaler. Happy to change that test something else

[TylerHelmuth]

Looking at the pro/con table I'm not convinced that a change to a struct at this time is necessary. I think enough custom components outside Contrib are currently using this package that the breaking change would be significant. We've not received any user feedback that casting to a string is tricky and we've identified that expectation in the godoc. My opinion is that we move forward with the type as a string.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (203ae9b) 90.25% compared to head (dc71a3e) 90.24%.
Report is 32 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9427      +/-   ##
==========================================
- Coverage   90.25%   90.24%   -0.02%     
==========================================
  Files         344      344              
  Lines       17932    17932              
==========================================
- Hits        16185    16182       -3     
- Misses       1419     1421       +2     
- Partials      328      329       +1     

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

[mx-psi]

I filed:

• [configopaque, confmap] Change yaml test by confmap test #9442 for moving the tests around
• Decide if configopaque.String should be a string or a type alias #9441 for continuing the discussion on string vs type alias. I want to do a poll to see what people think.

[andrzej-stencel]

My opinion is that the explicit syntax with a struct is easier to understand what's going on when you read the code, which is the majority of what we do.

Fixing issues like this is what the unstable phase is for - iron out not only the mistakes, but also the rough edges that might make the code harder to read. I'm all for code readability and if this improves it (it does in my opinion), I believe it's worth doing.

It's not the end of the world if we don't make this change of course 🙂 But I think the "product" will be better if we do.

[bogdandrutu]

@mx-psi should we add also an rc phase for this?

[mx-psi]

@bogdandrutu On #8975 we decided not to do more release candidates, and instead be more time in beta. The criteria to be able to pass from beta to stable are the ones listed on #9167 (but of course ultimately we need approval from all maintainers :)).

Is there anything specific that worries you about this package (additional to the things listed on #9427 (comment) )?

[mx-psi]

@open-telemetry/collector-approvers as discussed offline, I believe we can merge this, PTAL

[andrzej-stencel]

🎉 🚀

[mx-psi]

Will wait until next Wednesday to merge this to get more feedback