-
Notifications
You must be signed in to change notification settings - Fork 975
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
Closed
robsunday
wants to merge
7
commits into
open-telemetry:main
from
robsunday:distribution-node-support
+24
−7
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2bbf601
Added support for distribution config in ConfigProvider
robsunday 327123d
Merge branch 'main' into distribution-node-support
robsunday 14f1f75
TOC updated
robsunday 028c721
Lint fix
robsunday c4ad83b
Merge branch 'main' into distribution-node-support
robsunday fba1a7a
Update specification/configuration/api.md
robsunday 96a95c6
Merge branch 'main' into distribution-node-support
robsunday File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ weight: 1 | |
| * [ConfigProvider](#configprovider) | ||
| + [ConfigProvider operations](#configprovider-operations) | ||
| - [Get instrumentation config](#get-instrumentation-config) | ||
| - [Get distribution config](#get-distribution-config) | ||
| * [ConfigProperties](#configproperties) | ||
|
|
||
| <!-- tocstop --> | ||
|
|
@@ -36,19 +37,20 @@ It consists of the following main components: | |
| ### ConfigProvider | ||
|
|
||
| `ConfigProvider` provides access to configuration properties relevant to | ||
| instrumentation. | ||
| instrumentation and distribution. | ||
|
|
||
| Instrumentation libraries access `ConfigProvider` during | ||
| initialization. `ConfigProvider` may be passed as an argument to the | ||
| instrumentation library, or the instrumentation library may access it from a | ||
| central place. Thus, the API SHOULD provide a way to access a global | ||
| Instrumentation libraries and vendor's custom code can access `ConfigProvider` during | ||
| and after initialization. `ConfigProvider` may be passed as an argument, or may be accessed from a | ||
| central place. | ||
| Thus, the API SHOULD provide a way to access a global | ||
| default `ConfigProvider`, and set/register it. | ||
|
|
||
| #### ConfigProvider operations | ||
|
|
||
| The `ConfigProvider` MUST provide the following functions: | ||
|
|
||
| * [Get instrumentation config](#get-instrumentation-config) | ||
| * [Get distribution config](#get-distribution-config) | ||
|
|
||
| TODO: decide if additional operations are needed to improve API ergonomics | ||
|
|
||
|
|
@@ -64,6 +66,18 @@ If the `.instrumentation` node is not set, get instrumentation config MUST | |
| return nil, null, undefined or another language-specific idiomatic pattern | ||
| denoting empty. | ||
|
|
||
| ##### Get distribution config | ||
|
|
||
| Obtain configuration relevant to a distribution. | ||
|
|
||
| **Returns:** [`ConfigProperties`](#configproperties) representing | ||
| the [`.distribution`](https://github.com/open-telemetry/opentelemetry-configuration/blob/670901762dd5cce1eecee423b8660e69f71ef4be/examples/kitchen-sink.yaml#L438-L439) | ||
|
Member
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 link points to the .instrumentation node |
||
| configuration mapping node. | ||
|
|
||
| If the `.distribution` node is not set, get distribution config MUST | ||
| return nil, null, undefined or another language-specific idiomatic pattern | ||
| denoting empty. | ||
|
|
||
| ### ConfigProperties | ||
|
|
||
| `ConfigProperties` is a programmatic representation of a configuration mapping | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ConfigProviderintentionally limited the scope to the.instrumentation/developmentnode, 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
.distributionnode 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
parseandcreatemethods that allow a distribution to access the.distributionnode. For example in java: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for the delayed response—I was on vacation.
ConfigProviderexposes 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:
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.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.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.
Ad 2 - instead of dedicated
AgentListenerwe could use customDeclarativeConfigurationCustomizerProviderthat takesDistributionModelfromOpenTelemetryConfigurationModeland then stores it in some static field, so it can be used later on. I still think thatConfigProvidersolution is better in terms of clarity and standardization.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.
@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