New - Support distribution.solarwinds config node with per-key merging#549
Open
cleverchuk wants to merge 7 commits into
Open
New - Support distribution.solarwinds config node with per-key merging#549cleverchuk wants to merge 7 commits into
distribution.solarwinds config node with per-key merging#549cleverchuk wants to merge 7 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for reading SolarWinds agent configuration from a new top-level distribution.solarwinds node in sdk-config.yaml, with per-key merging against the existing instrumentation/development.java.solarwinds node (distribution takes precedence per key). The resolution logic is centralized in a new SolarwindsConfigResolver, and both the agent bootstrap/customization path and shared config customizer are updated to use it; additionally, logging output is enhanced to include thread name and a flaky JDBC test is stabilized.
Changes:
- Introduce
SolarwindsConfigResolverto resolve and mergedistribution.solarwinds+instrumentation/development.java.solarwinds. - Update
DeclarativeLoaderandSharedConfigCustomizerProviderto use the centralized resolver. - Update example/smoke YAML configs, enhance internal log formatting, and stabilize JDBC instrumentation tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| smoke-tests/sdk-config.yaml | Moves shared SolarWinds settings into distribution.solarwinds for smoke tests. |
| sdk-config.yaml | Adds distribution.solarwinds to the root example config and adds placeholder sections. |
| libs/shared/src/test/java/com/solarwinds/opentelemetry/extensions/config/provider/SharedConfigCustomizerProviderTest.java | Adds coverage for reading/precedence/merge behavior across distribution + instrumentation nodes. |
| libs/shared/src/main/java/com/solarwinds/opentelemetry/extensions/config/SolarwindsConfigResolver.java | New centralized resolver implementing per-key merge semantics. |
| libs/shared/src/main/java/com/solarwinds/opentelemetry/extensions/config/provider/SharedConfigCustomizerProvider.java | Switches to centralized resolver instead of hard-coded instrumentation-node traversal. |
| libs/logging/src/main/java/com/solarwinds/joboe/logging/Logger.java | Updates log label formatting to include thread name. |
| libs/core/src/main/java/com/solarwinds/joboe/core/rpc/RpcClient.java | Minor refactor to a method reference for async init submission. |
| instrumentation/jdbc/javaagent/src/test/java/com/solarwinds/opentelemetry/instrumentation/JdbcInstrumentationTest.java | Stabilizes test by reusing container and asserting via span polling instead of exact trace structure. |
| custom/src/test/java/com/solarwinds/opentelemetry/extensions/config/DeclarativeLoaderTest.java | Updates tests to match new declarative customization integration and distribution-node support. |
| custom/src/main/java/com/solarwinds/opentelemetry/extensions/config/DeclarativeLoader.java | Switches to DeclarativeConfigurationCustomizerProvider and resolves SolarWinds config via the new resolver. |
| * across languages in a multi-language deployment) and/or under the language-specific {@code | ||
| * instrumentation/development.java.solarwinds} node. When both are present they are merged per key: | ||
| * the distribution node takes precedence for any key it defines and the instrumentation node fills | ||
| * in the rest. A present-but-empty node is treated as absent. |
| - name: service.name | ||
| value: swi-test-app | ||
|
|
||
| # Provide shared config for multi-langauge deployment |
| - name: test.app | ||
| value: java-apm-smoke-test-v2 | ||
|
|
||
| # Provide shared config for multi-langauge deployment |
Comment on lines
34
to
36
| tracer_provider: | ||
| processors: | ||
|
|
Comment on lines
38
to
40
| meter_provider: | ||
| readers: | ||
|
|
Comment on lines
42
to
44
| logger_provider: | ||
| processors: | ||
|
|
Comment on lines
+139
to
+140
| // distribution.solarwinds is present but has no properties, so it is skipped; configuration is | ||
| // still loaded from the instrumentation node. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The agent can now read its configuration from the top-level
distribution.solarwindsnode insdk-config.yaml, making it usable in multi-language deployments where shared settings live outsidethe Java-specific
instrumentation/development.java.solarwindsnode. When both nodes are present,their keys are merged:
distributionwins for any key it defines, and theinstrumentationnodefills in the rest. The resolution logic is centralized in a new
SolarwindsConfigResolverclassconsumed by both
DeclarativeLoaderandSharedConfigCustomizerProvider. A flaky JDBCinstrumentation test is also fixed in this branch.
Changes
DeclarativeLoader— new integration pointDeclarativeLoaderpreviously implementedBeforeAgentListenerand obtained its config viaExtendedOpenTelemetry.getInstrumentationConfig("solarwinds"), which only ever resolved theinstrumentation/development.java.solarwindsnode. It now implementsDeclarativeConfigurationCustomizerProviderand registers a model customizer, which gives it directaccess to the full config tree and lets it resolve both nodes before the SDK is fully initialized.
SolarwindsConfigResolver— centralized resolution with mergingBoth
DeclarativeLoaderandSharedConfigCustomizerProviderpreviously duplicated the nodetraversal logic (
getStructured("distribution")…). The newSolarwindsConfigResolverclass inlibs/sharedis the single place that knows how to locate and combine the two nodes.Resolution rules:
distribution.solarwindspresent → use it directly.instrumentation/development.java.solarwindspresent → use it directly.MergedConfigPropertiesview that resolves each key fromdistributionfirst and falls back to
instrumentationfor keys not defined there.null(callers wrap this inObjects.requireNonNull).MergedConfigPropertiesis a private inner class that implementsDeclarativeConfigPropertiesbydelegating every getter to the primary node and falling back to the secondary node when the primary
returns
null.getPropertyKeys()returns the union of both nodes' key sets. An empty node(present but no keys) is honored — it defers every key to the other node rather than blocking it.
sdk-config.yamlupdatesShared settings (
agent.serviceKey,agent.collector,agent.logging) are moved frominstrumentation/development.java.solarwindsinto the newdistribution.solarwindsblock in boththe root and smoke-test config files, reflecting the intended multi-language sharing model. Empty
stubs for
tracer_provider.processors,meter_provider.readers, andlogger_provider.processorsare added for discoverability.
Thread name in APM log lines
The internal logger format now includes
[thread-name]between the log level and the message,making it easier to correlate agent-internal log output with application thread activity.
Flaky JDBC test fix
JdbcInstrumentationTestwas flaky because:leak into subsequent assertions.
waitAndAssertTraces, which pinned the exact number and grouping of traces,while the JDBC driver emits a variable number of setup statements on connection open.
The fix makes the container a static field (started once for the class) and calls
testing.clearData()after the connection is opened to discard driver-setup spans before the codeunder test runs. Assertions are rewritten to poll
testing.spans()with Awaitility and check onlythat a
rootINTERNALspan carryingsw.query_tagwas exported, without constraining the rest ofthe trace structure.
Test services data