Consider marking changes to config struct fields as non-breaking

[atoulme]

Component(s)

No response

Describe the issue you're reporting

See #17273 for origin of this query.

The issue consists in changing many fields across most components to make one or more field opaque, such as their value cannot be disclosed accidentally.

The changes initially were marked as enhancement, and most recently as breaking. The changes should have been flagged as breaking for any components past alpha stage as this document states changes to the Go API of any component must be accounted for as breaking changes.

[mx-psi]

Some ideas that were floated around on Slack (putting emojis in case we want to make a poll at some point):

Option A. 🎉 Have a separate changelog for Go API only changes

We create a separate changelog file that lists changes in the Go API that don't result in user facing changes for users of the Collector as a binary. The rationale for this is that most people don't care about Go API only changes and it can be confusing to have them on the main changelog.

The main downside of this option is that it complicates the contribution process while we don't know if people really care about the Go API at all.

Option B. ❤️ Have a separate changelog section for Go API only changes

Same as Option A, but instead of a separate file we can have a separate section on the main changelog for Go API only changes. This still complicates the contribution process, but it's a bit simpler at the expense of possibly being more confusing.

Option C. 🚀 Move configuration structs to internal packages on each component's module

We move the Config structs from all components to an internal package, so that they stop being part of the public API. This aligns the Go API stability with the usual stability concept. Creating config structs remains usable if using CreateDefaultConfig + Unmarshal to create a configuration struct. The concern here becomes a nonissue then, since this is no longer part of the public Go API. An open question is if we can still support all use cases if the configuration structures are internal (e.g. OpAMP).

Option D. 👀 Explicitly call out that we make no stability guarantees for Go API on contrib components

Change VERSIONING.md to state that Go API is not considered when it comes to stability guarantees. We won't list the Go API only changes on the changelog going forward.

[jpkrohling]

Preference for ❤️, although 🎉 is also acceptable. I'm not sure I fully understand 🚀: doesn't it mean that people can't change the config returned by CreateDefaultConfig? The interface returns a generic Config construct, and casting to an internal type wouldn't work, I suppose. Unmarshal would work, but that's really cumbersome for people who are creating the component programmatically, without an external config source.

That said, can you expand on how ❤️ complicates the contribution process? Wouldn't it work if we add a new field to the changelog YAMLs, stating whether that's a change affecting consumers of the API or users of the collector?

[djaglowski]

🚀: doesn't it mean that people can't change the config returned by CreateDefaultConfig? The interface returns a generic Config construct, and casting to an internal type wouldn't work, I suppose. Unmarshal would work, but that's really cumbersome for people who are creating the component programmatically, without an external config source.

This would be a mistake in my opinion. There are sensible reasons to directly modify the config, some of which are useful in this repo. e.g:

• Lifecycle tests
• Integration tests

[djaglowski]

cc: @open-telemetry/opamp-go-approvers

[mx-psi]

I'm not sure I fully understand rocket: doesn't it mean that people can't change the config returned by CreateDefaultConfig? The interface returns a generic Config construct, and casting to an internal type wouldn't work, I suppose. Unmarshal would work, but that's really cumbersome for people who are creating the component programmatically, without an external config source.

@jpkrohling 🚀 would mean that the only way to change the configuration is through Unmarshal indeed.

So e.g. instead of

f := loggingexporter.NewFactory()
cfg := f.CreateDefaultConfig()
c := cfg.(*loggingexporter.Config)
c.Verbosity = configtelemetry.LevelDetailed

one would now have to do

f := loggingexporter.NewFactory()
cfg := f.CreateDefaultConfig()
conf := confmap.NewFromStringMap(map[string]any{
    "verbosity": "detailed",
})
err := component.UnmarshalConfig(conf, cfg)
if err != nil {
    panic(err)
}

That said, can you expand on how heart complicates the contribution process? Wouldn't it work if we add a new field to the changelog YAMLs, stating whether that's a change affecting consumers of the API or users of the collector?

@jpkrohling I think it's a minimal nuisance, but we do ask contributors to keep in mind one more changelog type and to understand the nuances behind it (to be clear, I'd say ❤️ is my preferred option, although I would be okay with 🚀 as well).

rocket: doesn't it mean that people can't change the config returned by CreateDefaultConfig? The interface returns a generic Config construct, and casting to an internal type wouldn't work, I suppose. Unmarshal would work, but that's really cumbersome for people who are creating the component programmatically, without an external config source.

This would be a mistake in my opinion. There are sensible reasons to directly modify the config, some of which are useful in this repo. e.g:

• Lifecycle tests
• Integration tests

Both of those examples could be handled in the way I described above. There are still downsides (e.g. we turn some compile-time errors into runtime errors, and we make the API generally more cumbersome to use), some of which can be solved (e.g. adding some helper functions), but I would say 🚀 does not limit much what we can do.

[tigrannajaryan]

🚀 would mean that the only way to change the configuration is through Unmarshal indeed.

This means we are preventing the compiler from helping us to know we broke some code that depends on the config structure. It is not preventing the breakage. The dependent code will still need to make fixes by adjusting whatever the expect to get as a result of unmarshalling.

[tigrannajaryan]

IMO, there is 3 separate audiences that care about stability of the Collector and they care about different things.

One is end users. They don't care about API stability, they care about config YAML format and component behavior.

The second audience is component developers, who care about core API stability (e.g. pdata, consumer, etc) their component depends on. They don't care much about other component APIs or the config YAML format.

The third audience is Collector maintainers who care about all of these things.

Our changelog indeed is a mix of all of these concerns. I would vote for some form of changelog separation by audience, such that we inform the particular audience about only relevant things that impact them. This probably would be beneficial regardless of whether we think config struct should be part of public API of the component or no.

[mx-psi]

[:rocket:] is not preventing the breakage. The dependent code will still need to make fixes by adjusting whatever the expect to get as a result of unmarshalling.

If the change affects unmarshaling, then it has user-facing consequences, so it's not a Go API only change like #17273. The point of 🚀 is that it would align the changes that are breaking for end users with those that are breaking for developers: after it there would be no configuration breaking changes that are only breaking for developers.

[mx-psi]

PTAL at open-telemetry/opentelemetry-go-build-tools/pull/360 implementing Option B ❤️ since that one seems to have the most support and no opposition

[dmitryax]

I think open-telemetry/opentelemetry-go-build-tools#360 might be confusing for go instrumentation library since any change there is a library change. Also, most of the user-facing changes change the go API. Are we going to duplicate the entries?

In most cases, a breaking change to the user interface is a breaking change to Go API. Changes like #17273 are pretty rare. I think we can keep the collector changelog end-user centric, and if a user interface enhancement breaks the Go API, we add a suffix like [Breaking change to Go API] to particular entries under the Enhancements section.

[mx-psi]

I think open-telemetry/opentelemetry-go-build-tools#360 might be confusing for go instrumentation library since any change there is a library change. Also, most of the user-facing changes change the go API. Are we going to duplicate the entries?

My intention is that only OpenTelemetry Collector contrib would use this change_type, and only for library only changes. If you can think of a better name, could you comment on the PR?

I think we can keep the collector changelog end-user centric, and if a user interface enhancement breaks the Go API, we add a suffix like [Breaking change to Go API] under the Enhancements section.

I slightly like more the new change_type since that is machine-readable and easy to check on CI (as in, if a change breaks the Go API, we can eventually validate that it has the right changelog type).

[djaglowski]

Discussion in this thread has led me to change my opinion to Option A. In short, I believe Option B does not correctly model the information we should be conveying. If you voted on this issue, please consider reviewing the discussion.

I've opened an issue here proposing a solution that would support that option.

[tigrannajaryan]

I agree with @djaglowski. Having a separate changelog file is a bit easier to consume than a separate section in the same file.

[jpkrohling]

If you voted on this issue, please consider reviewing the discussion

I changed my vote to 🎉 .

[djaglowski]

Option A has been implemented in the build-tools repo and #24771 would set up this repo to use it.