-
Notifications
You must be signed in to change notification settings - Fork 38
Add Distribution section for vendor-specific settings #433
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
Changes from all commits
3723d1a
dadf22f
61cc781
6641412
f4a586f
966262e
32d9789
603f47d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -436,3 +436,6 @@ instrumentation/development: | |
| swift: | ||
| example: | ||
| property: "value" | ||
| distribution: | ||
| example: | ||
| property: "value" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,14 @@ properties: | |
| description: | | ||
| Configure instrumentation. | ||
| defaultBehavior: instrumentation defaults are used | ||
| distribution: | ||
|
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. Should this be distribution/development, and reference With stability coming soon, we had better be sure about anything which isn't marked experimental. Makes me nervous to have new concepts which will be stable out of the gate.
Contributor
Author
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 why this need to be marked as unstable, since there are no configurations for OpenTelemetry inside the node. Everything used there relies on vendor-specific configurations. The only thing that might be unstable on the OpenTelemetry side is the name of this node. Should we really mark it as
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 argument is that its a new concept that has not been leveraged anywhere. If we mark it stable and get it wrong, we're stuck with it forever. OpenTelemetry tends to be conservative. With that said, distributions are not a new concept in otel and we've been contending where to put distribution specific configuration for a while. I don't think we'll come with a different answer. |
||
| $ref: "#/$defs/Distribution" | ||
| description: | | ||
| Defines configuration parameters specific to a particular OpenTelemetry distribution or vendor. | ||
| This section provides a standardized location for distribution-specific settings | ||
| that are not part of the OpenTelemetry configuration model. | ||
| It allows vendors to expose their own extensions and general configuration options. | ||
| defaultBehavior: distribution defaults are used | ||
| required: | ||
| - file_format | ||
| $defs: | ||
|
|
@@ -92,3 +100,8 @@ $defs: | |
| $ref: resource.yaml | ||
| ExperimentalInstrumentation: | ||
| $ref: instrumentation.yaml | ||
| Distribution: | ||
| type: object | ||
| additionalProperties: | ||
| type: object | ||
| minProperties: 1 | ||
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.
is
examplethe name of a distribution here? if not, should it be? e.g. in Java Instrumentation we have at least two (upstream) distributionsagentandspring_starter, I could see it being helpful to know what kind of config yaml we're looking ator if not, maybe
distributioncould have a (mandatory?)name:?Uh oh!
There was an error while loading. Please reload this page.
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.
exampleis the name of a vendor. For examplesplunk,grafana,new_relicThere 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 agree that a
distributioncannot be empty and should have at least one vendor name inside it.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 pushed a commit that ensures this. It also ensures that each entry under distribution is an object, disallowing something like:
Has to be modeled like: