Skip to content
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.

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

[featuregate] Include the description, fromVersion, toVersion, and reference URL in String() #7625

Open
TylerHelmuth opened this issue May 3, 2023 · 2 comments

Comments

@TylerHelmuth
Copy link
Member

Is your feature request related to a problem? Please describe.
Although we set description, fromVersion, toVersion, and reference url in the gates today, users cannot see those values unless they use zpagesextension. Only the default value based on the stage is included in the flag's String() implementation. As a result, users can see the available flags when using the --help command, but not when the flags represent. This can cause confusion and extra burden on feature gate users to ensure the gate is documented somewhere else.

Describe the solution you'd like
Each feature gate's description, toVersion, fromVersion, and referenceURL is included in string so that users know what they can use.

Describe alternatives you've considered
zpagesextension can display all this information, but the collector must have that extension and be running in order to be useful.

@mx-psi
Copy link
Member

mx-psi commented Aug 11, 2023

Should this block featuregate 1.0? I don't think so, since the String method should not have any guarantees as to what its output is other than "human readable description of the feature gate".

@TylerHelmuth
Copy link
Member Author

I agree that this does not need to be implemented before a 1.0. It can be a feature add in a later version.

mx-psi added a commit that referenced this issue Dec 11, 2023
**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
Projects
None yet
Development

No branches or pull requests

2 participants