Use declarative config throughout dynamic-control - #3013
zeitlinger wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates dynamic-control internals to use DeclarativeConfigProperties as the common configuration interface, keeping ConfigProperties limited to the legacy auto-configuration compatibility boundary.
Changes:
- Switched policy source/provider wiring (
PolicyInit,SourceKind,OpampPolicyProvider) to consumeDeclarativeConfigPropertiesdirectly. - Removed the declarative-to-
ConfigPropertiesadapter/fallback layer from the declarative sampler component path. - Bridged legacy auto-configuration to declarative properties at activation time via
ConfigPropertiesBackedConfigProvider, and updated tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/TelemetryPolicySamplerComponentProvider.java | Removes ConfigProperties bridging/fallback from the declarative sampler component path. |
| dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/OpampPolicyProvider.java | Converts OpAMP provider configuration input to DeclarativeConfigProperties and updates header/resource attribute resolution. |
| dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/registry/PolicyInit.java | Bridges legacy auto-config properties to declarative properties when activating sources. |
| dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/registry/PolicyInitConfig.java | Refactors declarative parsing reuse and simplifies legacy YAML/JSON path selection/reading. |
| dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/source/SourceKind.java | Updates provider creation signature to accept DeclarativeConfigProperties. |
| dynamic-control/src/test/java/io/opentelemetry/contrib/dynamic/policy/OpampPolicyProviderTest.java | Updates unit tests to reflect declarative properties for OpAMP config/header/resource attributes. |
| dynamic-control/src/test/java/io/opentelemetry/contrib/dynamic/policy/registry/PolicyInitTest.java | Updates tests for the new initFromDeclarativeConfig signature. |
| dynamic-control/src/test/java/io/opentelemetry/contrib/dynamic/policy/source/SourceKindTest.java | Updates source kind/provider tests to use declarative properties. |
Comments suppressed due to low confidence (1)
dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/OpampPolicyProvider.java:360
getServiceEnvironmentassumesotel.resource.attributesis present. Ifproperties.get(RESOURCE_ATTRIBUTES)returns null, this will throw; previous behavior returned null when resource attributes were missing. Default toDeclarativeConfigProperties.empty()to preserve that behavior.
DeclarativeConfigProperties resourceAttributes = properties.get(RESOURCE_ATTRIBUTES);
String semconvEnvironment = resourceAttributes.getString(DEPLOYMENT_ENVIRONMENT_NAME);
|
I don't think this correctly handles the opamp provider. There is (as yet) no declarative config for opamp, so it needs to continue to be supported via properties. I'll have time to look at this next week |
3bf4c64 to
337dc08
Compare
|
Thanks, I updated this to keep OpAMP on the legacy properties path until it has a declarative schema. The legacy auto-configuration path still exposes the general flat configuration through Headers need a separate treatment: the declarative bridge documents that This keeps |
we could extend in different ways to bridge this gap - but this hasn't been needed in the Javaagent for which the bridge has been developed I think I'll try it out - just to see what it would look like (as a separate PR) |
To do that I'd have to know where in the declarative config schema "otel.experimental.opamp.headers" and "otel.resource.attributes" should be located. If they are not there at all, then the bridge would be more confusing than helpful in making it look like it could be found in declarative config. |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
ed1aaa1 to
304ce21
Compare
|
This PR targets main now. |
|
@zeitlinger okay for me to commit changes to the branch or would you prefer to see a diff and apply that yourself? |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
the PR already targets main - what do you mean? |
now I get it - yes, feel free to do any changes or create a new PR - whatever works best for you |
|
I couldn't push to your branch because of perms, so I created zeitlinger#1 |
|
@zeitlinger is there a problem with the suggested changes? |
I missed this - sorry - will look at it soon |
answered |
|
Blocked by open-telemetry/opentelemetry-java#7832 (or an equivalent resource-aware SDK callback). For declarative configuration, the OpAMP client should identify itself using the SDK's resolved Resource, including The intended flow is to prepare the sampler/policy state during configuration, then start OpAMP from the existing post-autoconfiguration callback once the SDK is built. That callback currently receives the SDK but cannot access its Resource. I've reopened #7832 to discuss exposing it with the SDK maintainers: open-telemetry/opentelemetry-java#7832 (comment). OpAMP endpoint and authentication-header settings can retain their temporary sysprop/env configuration while the OpAMP declarative schema is unsettled. The blocker here is obtaining the resolved service identity without an agent-specific workaround. I'll hold off on landing the OpAMP compatibility changes until we settle that API direction. |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
|
@jackshirazi here's my update - it's the best I can come up with - but we can discuss more in the SIG meeting User impact: configure the service name twiceWith this implementation, users need to configure This is a bad user experience: it duplicates configuration and risks OpAMP reporting a different identity from the telemetry it manages. The PR now implements and documents this workaround, so the migration no longer depends on the proposed SDK resource getter. Why the duplication is necessaryOpAMP needs the SDK’s resolved service identity. Simply copying The extension’s post-configuration callback receives the built SDK, but cannot access its resolved resource. Injecting system properties into the SDK’s declarative configuration would avoid some duplication, but would violate DC semantics rather than solve the resource-access problem. The implementation therefore keeps the two paths separate: the SDK and dynamic-control policies use declarative configuration, while OpAMP reads its endpoint, headers, and identity independently from properties or environment variables. Path to a better solutionAs Jack explained, he is not opposed to exposing this information; it needs a specification basis. The proposed entities I’d encourage the folks working on OpAMP to bring this concrete usability problem into the specification process and help advance the OpAMP/SDK integration. That would let us replace duplicate configuration with a consistent, supported solution. StatusThe workaround is implemented, tested, and documented in the README. It lets this migration proceed without making SDK resource access a prerequisite for agent 3.0. |
Blocked by open-telemetry/opentelemetry-java#7832
Summary
DeclarativeConfigPropertiesas the configuration interface for dynamic-control policy sourcesConfigPropertiesadapter from the sampler componentRationale
This follows the design discussion in #2989, while keeping
ConfigPropertiesout of the dynamic-control internals as the common configuration interface.We deliberately do not use the deprecated
DeclarativeConfigPropertiesBridgeBuilder. Its removal is slated for the upcoming 3.0 release, so this is more than a cleanup concern: using it would add a dependency on an API that will soon disappear and would keepConfigPropertiesas the unifying interface.Instead, the declarative component path passes
DeclarativeConfigPropertiesdirectly through dynamic-control:TelemetryPolicySamplerComponentProviderreceives declarative properties.PolicyInitandPolicyInitConfigconsume declarative properties directly.SourceKindandOpampPolicyProvideruse declarative properties internally.ConfigPropertiesadapter is created for the declarative component path.For the legacy auto-configuration path, we still need to support the existing file-based configuration:
Those properties point to legacy YAML/JSON files, so
readFromConfigPropertiesremains an intentional compatibility boundary. It is not used as the internal configuration abstraction, and it cannot be replaced by the declarative bridge without changing the legacy property names and file-loading behavior.At the point where legacy auto-configuration activates policy sources, we use the new
DeclarativeConfigBridgewith an empty component prefix to expose the general flat properties asDeclarativeConfigProperties. This is important for OpAMP: OpAMP does not yet have a declarative schema, and its endpoint, service identity, resource attributes, and headers must continue to come from properties. The small boundary compatibility view retains the map-shaped legacy resource and header properties that the scalar bridge cannot represent. The dynamic-control internals still receive only declarative properties.This also avoids the deprecated
ConfigPropertiesBackedConfigProvider, whose instrumentation-scoped view would not expose the general OpAMP properties.This supports the three intended execution paths:
Optional structured lookups use the nullable
getStructuredform where the configuration block may be absent.getis used only for properties whose presence is guaranteed or where the implementation supplies the required empty default.Testing
./gradlew --no-daemon --no-parallel :dynamic-control:spotlessApply :dynamic-control:checkmise run lintFixes #2989