-
Notifications
You must be signed in to change notification settings - Fork 932
Mark declarative config as stable #4568
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
base: main
Are you sure you want to change the base?
Mark declarative config as stable #4568
Conversation
This is ready for review, but SHOULD not be merged. Assuming we get the required approvals, I'll coordinate merging this with cutting the corresponding release of |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type
parameter in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#register-componentprovider is not well-defined.
Is it an enumeration? I guess that the intention is that these are "SDK extension plugin interface" names.
Side note (for sure not needed for stabilization):
Do we want to also add some "instrumentation plugin" which could be any object so that the instrumentation/distribution could parse and interpret after it is returned by https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/api.md#get-instrumentation-config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a crack at it by adding this guidance:
SDKs should represent
type
in a manner that is idiomatic for their language.
For example, a class literal, an enumeration, or similar.
See supported SDK extension plugins for the set of
supportedtype
values.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a lot better. But please create a separate PR for it 😉
PS. I hope you are OK that I made some comments here instead of creating new (spamming) GitHub issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the comments are great.
…y-specification into stabilize-declarative-config
whatever manner is idiomatic for the language. | ||
|
||
TODO: Add operation to update SDK components with new configuration for usage | ||
with OpAmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking here: #3771
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to resolve.
PS. You can consider adding
<!-- TODO: https://github.com/open-telemetry/opentelemetry-specification/issues/3771 -->
if it helps you in future.
fail fast) in accordance with | ||
initialization [error handling principles](../error-handling.md#basic-error-handling-principles). | ||
|
||
TODO: define behavior if some portion of configuration model is not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have guidance around misalignment of file_format
specified by user and supported by an SDK: https://github.com/open-telemetry/opentelemetry-configuration?tab=readme-ov-file#file-format
Also related, we're working on tooling to allow implementations to document what properties they support here: open-telemetry/opentelemetry-configuration#197
* [LoggerProvider](../logs/sdk.md#loggerprovider) | ||
* [Propagators](../context/api-propagators.md#composite-propagator) | ||
* [ConfigProvider](#configprovider) | ||
* [ConfigProvider](#configprovider) (**Status**: [Development](../document-status.md)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just to follow the existing pattern from other places of the specification
* [ConfigProvider](#configprovider) (**Status**: [Development](../document-status.md)) | |
* **Status**: [Development](../document-status.md) - [ConfigProvider](#configprovider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. I recently was talking with internal colleagues of mine who were reading the log spec and thought that the a variety of stable components were "In Development" because of confusing placement of the status text. For example:

Because there is no other text in the Logger section, and because its not clear where the "In development" ends, the entire SDK Logger component seems to be unstable, which of course is not the case.
I do think we should be consistent, but think that spec's current strategy for calling out a different status should be improved. Though in this particular case putting the status before the list entry doesn't seem to be more confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though in this particular case putting the status before the list entry doesn't seem to be more confusing.
I agree therefore I prefer consistency.
that spec's current strategy for calling out a different status should be improved
I also agree but this should be a subject of a separate PR.
I think we could consider making some sections like (using https://github.com/orgs/community/discussions/16925):
Note
Status: Development
Paragraph. Tell me something more.
Not sure how it will render in https://opentelemetry.io/.
@open-telemetry/docs-approvers, do you have any ideas?
For bullet-points, I think that the current strategy may be good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answering for the @open-telemetry/docs-approvers, it's a known issue that GitHub Markdown alerts don't render well on opentelemetry.io. That being said, there are spec docs that use this notation today.
* `component_provider` - The `ComponentProvider`. | ||
* `type` - The type of plugin interface it provides (e.g. SpanExporter, Sampler, | ||
etc). | ||
* `type` - The type of plugin interface it provides. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like that in one place we are saying "component" in other "plugin" or "plugin interfaces".
I know that it keeps the same intention, but it makes me harder to read and interpret the specification.
I propose to use the term "Component" everywhere and just mention in the "Component" definition that this is a plugin that allows extensibility of the Configuration SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine with me. I was trying to connect the dots because elsewhere in the spec we both the words component and plugin interface. I'll try to be consistent here with "component" but also have a callout with something to the effect of: "... (also called plugin interface elsewhere in the spec)"
* `component_provider` - The `ComponentProvider`. | ||
* `type` - The type of plugin interface it provides (e.g. SpanExporter, Sampler, | ||
etc). | ||
* `type` - The type of plugin interface it provides. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#supported-sdk-extension-plugins should define "mappings" between a "type" and where it is supposed to be used in the configuration model e.g. that SpanProcessor
component is represented in https://github.com/open-telemetry/opentelemetry-configuration/blob/d6b127882f41db92b26017c736a52ceb6f2b4451/schema/tracer_provider.json#L208
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that linking would be helpful 👍
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
A concern I have is that this sort of implies that a spec for the behavior of Can the spec of config be stable before SDK initialization is defined? |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Is there anything blocking this PR? |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Re opened. Please look at the merge conflict on the spec compliance matrix, due to recent changes to yaml. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Resolves #4374.
We recently cut a release candidate for
opentelemetry-configuration
. After some additional bake-in time and review, we'd like to cut a stable release.The stable release needs to coincide with marking key parts of the specification stable, so I'm opening this PR to solicit feedback such that when the time comes, we can mark the specification and
opentelemetry-configuration
stable is quick succession.See open-telemetry/opentelemetry-configuration#100 for status of implementations.
cc @open-telemetry/configuration-maintainers