Skip to content

Document Go API stability guarantees #8002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mx-psi opened this issue Jun 29, 2023 · 2 comments · Fixed by #8812
Closed

Document Go API stability guarantees #8002

mx-psi opened this issue Jun 29, 2023 · 2 comments · Fixed by #8812
Assignees
@mx-psi
Labels
area:config discussion-needed Community discussion needed priority:p1 High release:required-for-ga Must be resolved before GA release

Comments

@mx-psi
Copy link
Member

mx-psi commented Jun 29, 2023

To start marking more modules as stable, we want to discuss the following open questions about configuration structs:

  1. Does adding new validation rules mean a breaking change?
  2. Should we offer a "NewDefault...." for each config and say that the behavior of the config is stable only if initialized with NewDefault?
  3. Is adding new fields means breaking change? Let's assume someone "squash" the message into another message, then adding a field with same mapstruct value will be a breaking change, what do we do with that? What are the rules we follow.

We generally also need to think of any special implications for configs that are not defined by the golang conventions of breaking compatibility.

Reworded from post by @bogdandrutu in #7954 (comment)
@mx-psi mx-psi added discussion-needed Community discussion needed release:required-for-ga Must be resolved before GA release priority:p1 High area:config labels Jun 29, 2023
@mx-psi
Copy link
Member Author

mx-psi commented Jun 29, 2023

For (3), we already have some wording on https://github.com/open-telemetry/opentelemetry-collector/blob/v0.80.0/VERSIONING.md:

additive changes to the set of exported fields in a configuration structure are not considered to break backward compatibility.

Adding methods is also considered fine (see 'Methods' on the last section of that doc).

@mx-psi mx-psi mentioned this issue Jun 29, 2023
Closed
@mx-psi mx-psi mentioned this issue Nov 7, 2023
Merged
@mx-psi
Copy link
Member Author

mx-psi commented Nov 7, 2023

#8812 intends to fix this

@mx-psi mx-psi self-assigned this Nov 7, 2023
mx-psi added a commit that referenced this issue Dec 11, 2023
@mx-psi @songy23
[VERSIONING.md] Reorganize and clarify compatibility guarantees (#8812)
a5cb92a 
**Description:** Reorganize `VERSIONING.md`, specify compatibility
guarantees that have already been discussed and answer questions on
#8002.

Three new guarantees in the document had already been discussed
elsewhere:

- String representation:
#7625 (comment)
- Go version compatibility: Mentioned [on
README.md](https://github.com/open-telemetry/opentelemetry-collector?tab=readme-ov-file#compatibility)
- Dependency updates: Discussed in private, most recently in relation to
whether #7095 should block `pdata`'s 1.0 (consensus seems to be that it
should not).

Regarding the questions mentioned on #8002:

> Does adding new validation rules mean a breaking change?

Generally, yes except when the configuration was already invalid (i.e.
the Collector did not work properly with it).

> Should we offer a "NewDefault...." for each config and say that the
behavior of the config is stable only if initialized with NewDefault?

I did not add anything for this one. I think we should discuss it, but:
- I don't think there is a sensible output for `NewDefault...` for all
configurations (e.g. for `confignet.TCPAddr`, what would the default
be?)
- It seems to me that we should handle configuration validity through
`Validate`, and not through `NewDefault...`. `NewDefault` may help but
ultimately we need to verify through `Validate`.

> Is adding new fields means breaking change? Let's assume someone
"squash" the message into another message, then adding a field with same
mapstruct value will be a breaking change, what do we do with that? What
are the rules we follow.

This was already in this document, where we had decided that adding new
fields was okay. I think it would be too stringent to bar us from adding
new fields.

**Link to tracking Issue:** Fixes #8002

---------

Co-authored-by: Yang Song <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mx-psi mx-psi

Labels
area:config discussion-needed Community discussion needed priority:p1 High release:required-for-ga Must be resolved before GA release
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[VERSIONING.md] Reorganize and clarify compatibility guarantees mx-psi/opentelemetry-collector
1 participant
@mx-psi