-
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?
Changes from all commits
04649ba
274afc5
678225d
e1a7e09
72946fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Side note (for sure not needed for stabilization): There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took a crack at it by adding this guidance:
Let me know what you think. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yes the comments are great. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,7 +5,7 @@ weight: 3 | |||||
|
||||||
# Configuration SDK | ||||||
|
||||||
**Status**: [Development](../document-status.md) | ||||||
**Status**: [Stable](../document-status.md) except where otherwise specified | ||||||
|
||||||
<!-- toc --> | ||||||
|
||||||
|
@@ -23,7 +23,7 @@ weight: 3 | |||||
+ [Register ComponentProvider](#register-componentprovider) | ||||||
* [Examples](#examples) | ||||||
+ [Via configuration API](#via-configuration-api) | ||||||
+ [Via OTEL_EXPERIMENTAL_CONFIG_FILE](#via-otel_experimental_config_file) | ||||||
+ [Via OTEL_CONFIG_FILE](#via-otel_config_file) | ||||||
* [References](#references) | ||||||
|
||||||
<!-- tocstop --> | ||||||
|
@@ -62,6 +62,8 @@ the name `Configuration` is RECOMMENDED. | |||||
|
||||||
### ConfigProvider | ||||||
|
||||||
**Status**: [Development](../document-status.md) | ||||||
jack-berg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
The SDK implementation of [`ConfigProvider`](./api.md#configprovider) MUST be | ||||||
created using a [`ConfigProperties`](./api.md#configproperties) representing | ||||||
the [`.instrumentation`](https://github.com/open-telemetry/opentelemetry-configuration/blob/670901762dd5cce1eecee423b8660e69f71ef4be/examples/kitchen-sink.yaml#L438-L439) | ||||||
|
@@ -189,9 +191,6 @@ Note: Because these operations are stateless pure functions, they are not | |||||
defined as part of any type, class, interface, etc. SDKs may organize them in | ||||||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to resolve. <!-- TODO: https://github.com/open-telemetry/opentelemetry-specification/issues/3771 --> if it helps you in future. |
||||||
|
||||||
#### Parse | ||||||
|
||||||
Parse and validate a [configuration file](./data-model.md#file-based-configuration-model). | ||||||
|
@@ -255,7 +254,7 @@ Interpret configuration model and return SDK components. | |||||
* [MeterProvider](../metrics/sdk.md#meterprovider) | ||||||
* [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 commentThe 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
I agree therefore I prefer consistency.
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/. 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 commentThe 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. |
||||||
|
||||||
The multiple responses MAY be returned using a tuple, or some other data | ||||||
structure encapsulating the components. | ||||||
|
@@ -284,8 +283,6 @@ This SHOULD return an error if it encounters an error in `configuration` (i.e. | |||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. We now have guidance around misalignment of Also related, we're working on tooling to allow implementations to document what properties they support here: open-telemetry/opentelemetry-configuration#197 |
||||||
|
||||||
#### Register ComponentProvider | ||||||
|
||||||
The SDK MUST provide a mechanism to | ||||||
|
@@ -298,15 +295,19 @@ as `ComponentProvider`s. | |||||
**Parameters:** | ||||||
|
||||||
* `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 commentThe 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". There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that linking would be helpful 👍 |
||||||
* `name` - The name used to identify the type of component. This is used | ||||||
in [configuration model](./data-model.md) to specify that the | ||||||
corresponding `component_provider` is to provide the component. | ||||||
|
||||||
The `type` and `name` comprise a unique key. Register MUST return an error if it | ||||||
is called multiple times with the same `type` and `name` combination. | ||||||
|
||||||
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](#sdk-extension-components) for the set of | ||||||
supported `type` values. | ||||||
|
||||||
### Examples | ||||||
|
||||||
#### Via configuration API | ||||||
|
@@ -365,10 +366,10 @@ ContextPropagators propagators = openTelemetry.getPropagators(); | |||||
ConfigProvider configProvider = openTelemetry.getConfigProvider(); | ||||||
``` | ||||||
|
||||||
#### Via OTEL_EXPERIMENTAL_CONFIG_FILE | ||||||
#### Via OTEL_CONFIG_FILE | ||||||
|
||||||
Setting | ||||||
the [OTEL_EXPERIMENTAL_CONFIG_FILE](./sdk-environment-variables.md#declarative-configuration) | ||||||
the [OTEL_CONFIG_FILE](./sdk-environment-variables.md#declarative-configuration) | ||||||
environment variable (for languages that support it) provides users a convenient | ||||||
way to initialize OpenTelemetry components without needing to learn | ||||||
language-specific configuration details or use a large number of environment | ||||||
|
@@ -378,11 +379,11 @@ resemble: | |||||
|
||||||
```shell | ||||||
# Set the required env var to the location of the configuration file | ||||||
export OTEL_EXPERIMENTAL_CONFIG_FILE="/app/sdk-config.yaml" | ||||||
export OTEL_CONFIG_FILE="/app/sdk-config.yaml" | ||||||
``` | ||||||
|
||||||
```java | ||||||
// Initialize SDK using autoconfigure model, which recognizes that OTEL_EXPERIMENTAL_CONFIG_FILE is set and configures the SDK accordingly | ||||||
// Initialize SDK using autoconfigure model, which recognizes that OTEL_CONFIG_FILE is set and configures the SDK accordingly | ||||||
OpenTelemetry openTelemetry = AutoConfiguredOpenTelemetrySdk.initialize().getOpenTelemetrySdk(); | ||||||
|
||||||
// Access SDK components and install instrumentation | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.