Added support for distribution config in ConfigProvider#4800
Added support for distribution config in ConfigProvider#4800robsunday wants to merge 7 commits intoopen-telemetry:mainfrom
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.
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.
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.
There was a problem hiding this comment.
I'm sorry for the delayed response—I was on vacation.
ConfigProvider exposes instrumentation configuration in a clear and consistent way for all languages. I wanted the distribution configuration to be exposed consistently among languages as well. I believe this would be very beneficial, which is why this PR was created.
Regarding the recommended alternative approach, I see some issues with it:
- It does not really improve security over the
ConfigProviderextension. Extracting secrets from the declarative config file is still possible when calling theparsemethod, right? At the same time, it complicates life for honest developers. - The Java example above is problematic for Otel Java Agent-based distributions that need distribution configuration settings in AgentListeners. The SDK is configured from
AgentInstallerwithout any extension hooks that could be used, so the config file would have to be parsed again in each listener to extractDistributionModel. Of course, we could create a dedicatedAgentListenerthat parses the config and stores it in some static variable, but in my opinion, that starts to get tricky. - It will lead to inconsistent implementations among languages.
There was a problem hiding this comment.
Ad 2 - instead of dedicated AgentListener we could use custom DeclarativeConfigurationCustomizerProvider that takes DistributionModel from OpenTelemetryConfigurationModel and then stores it in some static field, so it can be used later on. I still think that ConfigProvider solution is better in terms of clarity and standardization.
There was a problem hiding this comment.
@robsunday I have implemented exactly what you suggested - and only saw your comment now.
My takeaway was also that it starts to get tricky with statics - see open-telemetry/opentelemetry-java-instrumentation#15822
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
|
@robsunday could you please work with Java SIG on unblocking your work and trying out alternative proposals? |
I think @jack-berg's SDK / API separation argument is good, and I believe his example of how we can access distribution configuration should work for the OpenTelemetry Java agent distribution. We can always come back if we run into further issues.
Yes, however I still have some comments for this PR |
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