-
Notifications
You must be signed in to change notification settings - Fork 941
Added support for distribution config in ConfigProvider #4800
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?
Added support for distribution config in ConfigProvider #4800
Conversation
|
cc @jack-berg |
| Obtain configuration relevant to vendor specific distribution. | ||
|
|
||
| **Returns:** [`ConfigProperties`](#configproperties) representing | ||
| the [`.distribution`](https://github.com/open-telemetry/opentelemetry-configuration/blob/670901762dd5cce1eecee423b8660e69f71ef4be/examples/kitchen-sink.yaml#L438-L439) |
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 link points to the .instrumentation node
| The `ConfigProvider` MUST provide the following functions: | ||
|
|
||
| * [Get instrumentation config](#get-instrumentation-config) | ||
| * [Get distribution config](#get-distribution-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.
All instrumentation will have access to this. The original design of ConfigProvider intentionally limited the scope to the .instrumentation/development node, so as to avoid the questions about whether its appropriate / safe for instrumentation to have access to the SDK config model, including any secrets that may be present on the exporter nodes.
If we want to broaden scope, we need to have that conversation since its very reasonable that .distribution node will have the types of secrets that are in the SDK config model.
On a related note, why is this necessary?
The declarative config has separate parse and create methods that allow a distribution to access the .distribution node. For example in java:
OpenTelemetryConfigurationModel model = parse(new File(System.getEnv("OTEL_EXPERIMENTAL_CONFIG_FILE")));
DistributionModel distribution = model.getDistribution();
OpenTelemetrySdk sdk = create(model);
If there's not a strong need for this, the safe answer is to continue the status quo and not grant instrumentation access to more config than they need.
jack-berg
left a comment
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.
Requesting changes because I want to make sure we discuss this before merging.
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Fixes #4770
This PR introduces Declarative Configuration support for distribution specific configuration.
The
.distributionnode is now available in the root of Declarative Configuration YAML file, as agreed on SIG.Changes
ConfigProvider is extended with new operation to provide access to the
.distributionnodeFor non-trivial changes, follow the change proposal process.
CHANGELOG.mdfile updated for non-trivial changes