make declarative config bridge usable by spring starter and contrib #14497
Conversation
|
🔧 The result from generateLicenseReport was committed to the PR branch. |
|
❌ The result from spotlessApply could not be committed to the PR branch, see logs: https://github.com/open-telemetry/opentelemetry-java-instrumentation/actions/runs/17149887945. |
|
got the agent jar has this - so I don't understand why the class won't load
I think I'd have to remove the bridle from bootstrap here - but that only works for entire modules @laurit do you have an idea? |
|
🔧 The result from spotlessApply was committed to the PR branch. |
|
It worked! @laurit can you check if this makes sense? |
|
@trask here's the bridge moved to the agent repo |
| The auto configuration **with declarative config** uses the Declarative Config Bridge to be able to | ||
| use common configuration method: |
There was a problem hiding this comment.
Could be worth adding for extra clarity that this allows to use declarative/yaml configuration in addition to the java properties and environment variables, in short we have to change how configuration is consumed to support both, not how it's being set.
There was a problem hiding this comment.
no, if you use declarative config you can't use env vars any more
There was a problem hiding this comment.
Do the java properties still work ? Also, do we still have a way to handle compatibility with existing dotted syntax even when configuration options do not strictly fit yaml structure ? (disclamer: I have little knowledge about declarative configuration, maybe this is documented somewhere).
There was a problem hiding this comment.
Do the java properties still work ?
Not by default - but you can add that capability back.
The migration schema provides the same capabilities as before, e.g. value: ${OTEL_SERVICE_NAME:-unknown_service} adds support for the env var and sys property for the service name
Also, do we still have a way to handle compatibility with existing dotted syntax even when configuration options do not strictly fit yaml structure ?
- this PR allows existing instrumentations to read simple values or lists from either yaml or sys props
- if a new instrumentation needs to access data that only fits in yaml, then it can't use the bridge - e.g. here
InstrumentationConfig config = AgentInstrumentationConfig.get();
List<TypeInstrumentation> list =
config.isDeclarative()
? MethodsConfig.parseDeclarativeConfig(config.getDeclarativeConfig("methods")) <-- this has access to yaml tree
: parseConfigProperties();(disclamer: I have little knowledge about declarative configuration, maybe this is documented somewhere).
no worries - the user facing documentation is not there yet
There was a problem hiding this comment.
The only thing that I worry (at least for now) is having the capability to preserve the current support for environment variables and JVM system properties in a distribution (also includes some elements from contrib), so any documentation how to achieve this would be welcome. If users decide to use declarative configuration, then it's a breaking change and we don't have such requirement (and they should be able to use system properties and env variables in yaml if needed).
There was a problem hiding this comment.
we're not changing anything for user that are not using declarative config
| exclude("io/opentelemetry/instrumentation/api/incubator/config/**") | ||
| exclude("io/opentelemetry/instrumentation/api/incubator/instrumenter/**") | ||
| exclude("io/opentelemetry/instrumentation/api/incubator/log/**") | ||
| exclude("io/opentelemetry/instrumentation/api/incubator/semconv/**") |
There was a problem hiding this comment.
if a new package was added and we forgot to add an exclusion for it here, would something fail in a way we'd know to add it?
There was a problem hiding this comment.
I believe this would fail
It failed for me until I changed this at least.
There was a problem hiding this comment.
cool, i was wondering how that change was related
No description provided.