-
Notifications
You must be signed in to change notification settings - Fork 260
add known methods for declarative configuration #2504
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?
Conversation
@jack-berg I didn't see any mention of other declarative configuration properties in this repo. Should they be added? |
This is an unsolved problem. For now, I did a one time analysis of the semantic conventions repo and modeled of the concepts that are supposed to be configurable as properties in the declarative configuration schema: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema/instrumentation.json#L50-L128 @lmolkova has previously expressed the idea that the I like this idea but for now its just an idea. |
I also like that. |
related: #1450 How I see it:
I don't believe anyone is working on it, but I would be exited to see any detailed suggestions/proposals in this space. |
@jack-berg what are your thoughts on semconv eventually owning the instrumentation config schema - it'd be difficult to write and implement conventions otherwise having config defined separately. Our long-term vision is that codegen will get smarter and would generate whole spans/events/etc according to configuration, so we'd need to define/refer to config properties formally here in semconv and would interact with config API to retrieve values. |
I'm supportive. We can find a way to make |
I propose we make this transformation separate from adding the property now - to avoid delaying the feature. I've created open-telemetry/opentelemetry-configuration#244 |
58310b9
to
e224308
Compare
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.
Thanks! I left some comments. A few additional thoughts:
- this is a first precedent documenting declarative config in semantic conventions, which I support
- we need to find way to record config options formally in YAML schema at least in some cases, but it's not trivial and we need to collect more precedents to start formalizing it. I don't want to block this PR on that. But we should leave some TODOs around options being introduced and link #1450 as tracking issue
- I'd like to hear what other @open-telemetry/specs-semconv-maintainers think. @zeitlinger could you please bring this topic up on the next semconv call? Thanks!
model/peer/registry.yaml
Outdated
@@ -13,3 +13,18 @@ groups: | |||
of the remote service. SHOULD be equal to the actual `service.name` | |||
resource attribute of the remote service if any. | |||
examples: "AuthTokenCache" | |||
note: > | |||
It MUST be possible to configure the peer service name in declarative configuration as follows: |
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.
do we know which component would be responsible to use this configuration? This requirement should be actionable for this component. E.g. specialized span or log processor would match network.protocol.address
to peer
and populate peer.service
on all spans based on the service
.
In the current form, this requirement is only actionable for declarative config schema.
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.
- yes, it should only apply to declarative config, because this is a use case that is too complicated for env vars
- the responsible component is typically a shared library in the instrumentation part
- specialized log processor: that does not work because you typically don't have access to the URL for HTTP server duration
model/peer/registry.yaml
Outdated
@@ -13,3 +13,18 @@ groups: | |||
of the remote service. SHOULD be equal to the actual `service.name` | |||
resource attribute of the remote service if any. | |||
examples: "AuthTokenCache" | |||
note: > | |||
It MUST be possible to configure the peer service name in declarative configuration as follows: |
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.
please add Development
status for this note
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 whole attribute has Development
stability
- peer: 1.2.3.4 | ||
service: FooService | ||
- peer: 2.3.4.5 | ||
service: BarService |
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 this format defined in the declarative config? I'd use exact attribute names instead.
- peer: 1.2.3.4 | |
service: FooService | |
- peer: 2.3.4.5 | |
service: BarService | |
- network.peer.address: 1.2.3.4 | |
peer.service: FooService | |
- network.peer.address: 2.3.4.5 | |
peer.service: BarService |
but also, it could be much more generic and allow to do things like "if attribute A matches value B, then set attribute C with value D"
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.
just added the following description:
- `peer` is host, and optionally the and path of the remote service that is used to identify the service
instance, e.g., `example.com:443`, `1.2.3.4:8080`, `example.com`, `example.com:443/fooService`.
- `service` is the name of the remote service, e.g., `AuthService` that will be translated to the
`peer.service` attribute.
network.peer.address
would be wrong, because it can contain the port and path as well- declarative config uses
_
style it could be much more generic
- this has to be implemented in every SDK, so we try to limit such complex features
- otel collector has an attributes processor where you can already do this if needed
peer_service
would still be an option
**[1] `peer.service`:** It MUST be possible to configure the peer service name in declarative configuration as follows: | ||
|
||
```yaml | ||
instrumentation/development: |
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.
why would it be an instrumentation responsibility at all BTW? There are a lot of HTTP / RPC/ etc instrumentations producing spans that may benefit from peer.service
.
This enrichment can be done in the single processor (collector or per-language) and dozens of instrumentations would never bother.
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 SDK does not have access to the URL - see above
Co-authored-by: Liudmila Molkova <[email protected]>
Relates open-telemetry/opentelemetry-java#7468