Define instrumentation configuration API #4128
Conversation
There was a problem hiding this comment.
I think the text needs to be sanitized to avoid references to "file", especially in the API sections. API has nothing to do with files, it is only concerned with the configuration data model. Whether that configuration comes from a file or from a REST endpoint is irrelevant to the API.
What do folks think about breaking up
Do we think the spec would benefit from this decoupling? Or does the benefit of having all the information co-located in a single page outweigh the benefit of breaking out separate pages? |
I support breaking things down with a caveat:
talks about property structure/names without specifying where properties came from (file or not) in the same way as Spring configuration properties are not tight to any specific source. Having all things (data model, api, sdk) in one file could also be an option if file-based configuration details are defined in a different file. |
|
I've pushed a few commits which restructure the config docs. See updated PR description for details. |
There was a problem hiding this comment.
This feature is near and dear to the LLM work to me, as there are some configuration some vendors would like to be able to set, knowing there is a moratorium on OTEL_ values. For example, disabling of prompt/completion (think request/response) logging
I'm not an approver, but I'm going to play as one. I haven't reviewed fully the implementation, but I agree with the direction and consider my comments in the nit/as you wish category
|
After reviewing the impl something came to mind. We have our semantics, makes sense. Do we consider things that aren't generic enough to have their own semantics but ought to be common for all langs? e.g. openai specific config which may be an extension of genai/llm? or postgres config which ought to be the same regardless of java or whatever? Just clarifying end state scope interest. |
This is highly encouraged and very helpful 🙂 A handful of the comments were on sections of code which were relocated rather than introduced in this PR, but I addressed them anyhow because they were editorial.
I think so. Another example of this is configuration for opentelemetry-sqlcommenter - #3560 is tracking adding standardizing configuration for the various language implementations. Over in |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
I wonder about the autoconfig bridge from the java sdk to the spring starter and how it relates to this spec. Is such bridge function a may or should - or maybe something else entirely? |
My gut feeling is that this is a java specific concern, but I think we'll need to do some prototyping and see if anything emerges that seems like it will recur across languages. |
@brunobat is quarkus planning to use the config bridge? |
|
This PR has been open for over a month now and has enough approvals to merge. Will merge tomorrow unless someone communicates an intent to provide additional review / feedback. Thanks. |
No, Quarkus has no plans to use the yaml based config |
| undefined_key: ${UNDEFINED_KEY} # Invalid reference, UNDEFINED_KEY is not defined and is replaced with "" | ||
| ${STRING_VALUE}: value # Invalid reference, substitution is not valid in mapping keys and reference is ignored | ||
| recursive_key: ${REPLACE_ME} # Valid reference to REPLACE_ME | ||
| # invalid_identifier_key: ${STRING_VALUE:?error} # If uncommented, this is an invalid identifier, it would fail to parse |
There was a problem hiding this comment.
@jack-berg why isn't :?error syntax supported? It's standard shell syntax, just like :-default. Was there a discussion?
Bigger point - if we're using some convention, such as shell var syntax, it's very surprising when that convention is only partially supported. Context: https://github.com/open-telemetry/opentelemetry-collector/pull/10907/files#r1722093779
There was a problem hiding this comment.
Let's open an issue to track. This syntax wasn't introduced in this PR, it was just moved around. At the time when env var substitution syntax was needed, we were solving a very targeted problem and used shell syntax prior art to avoid reinventing something. You make a good point that partially supporting shell syntax would be surprising to some users.
Resolves #3535.
This introduces an API component to file configuration, which has been limited to SDK (i.e. end user facing) up until this point.
The configuration model recently added the first surface area related to instrumentation configuration properties in open-telemetry/opentelemetry-configuration#91.
The API proposed in this PR is collectively called the "Instrumentation config API", and provides a mechanism for instrumentation libraries to participate in file configuration and read relevant properties during initialization. The intent is for both OpenTelemetry-authored and native instrumentation alike to be able to be configured by users in a standard way. New API surface area is necessary to accomplish this to avoid instrumentation libraries from needing to take a dependency on SDK artifacts.
The following summarizes the additions:
.instrumentationnode defined in a config file.api.md,data-model.md, andsdk.mdfiles for respective API, data model, and SDK portions of declarative configuration. This aligns with other portions of the spec. The separation should improve clarity regarding what should and should not be exposed in the API.I've prototyped this new API in
opentelemetry-javahere: open-telemetry/opentelemetry-java#6549cc @open-telemetry/configuration-maintainers, @open-telemetry/specs-semconv-maintainers