You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Internal - Factor proxy config to individual components - #662
This pull request introduces several significant changes to the configuration handling and related components in the SolarWinds OpenTelemetry extensions. The main focus is on simplifying and improving the declarative configuration loading process, enhancing test coverage, and refactoring proxy and reporter handling. Notably, it removes the ReporterProvider class, updates how configuration is loaded and merged, and refactors proxy configuration parsing to use a shared helper. Additionally, the tests for declarative configuration have been updated to reflect these changes.
Configuration Loading and Management:
Refactored DeclarativeLoader to directly extract and merge configuration from the distribution and solarwinds nodes, removing the SolarwindsConfigResolver and improving robustness when no configuration is present. The configuration is now initialized via ConfigManager.initialize, and error handling has been simplified. [1][2]
Updated test cases in DeclarativeLoaderTest to match the new configuration logic. Tests now check for logging level configuration instead of service key/collector, ensure the loader does not throw on empty models, and verify that configuration merges with previously set values. [1][2][3]
Proxy Configuration Refactoring:
Replaced inline proxy parsing logic in HttpSamplerComponentProvider and ProfilingSpanProcessorComponentProvider with the shared ProxyHelper.toProxyConfig method, ensuring consistent and reusable proxy configuration handling. [1][2][3][4][5][6][7][8]
Reporter and Shutdown Handling:
Removed the ReporterProvider class and its usage, simplifying shutdown logic by only closing client managers and eliminating redundant event reporter handling. [1][2]
Dependency and Import Updates:
Added libs:logging as a test dependency in the Gradle build file.
Cleaned up imports to remove unused classes and ensure consistency with the new configuration approach. [1][2]
These changes collectively modernize and streamline configuration management, improve code clarity, and enhance test reliability.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The starting configuration exports no telemetry after exporter injection was removed, while full reloads retain deleted settings and declarative profiling loses authenticated-proxy support.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The smoke-test assertion can pass when the sampler bypasses the proxy because profiler traffic sets the same flag, leaving sampler-proxy regressions undetected.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Smoke test can pass without sampler traffic using the proxy
This assertion can pass even when the sampler bypasses the proxy. smoke-tests/sdk-config.yaml also enables swo_profiler with the same collector and Squid proxy, so a profiling connection alone sets this flag. Isolate sampler traffic using a separate proxy instance/port, or check it in a run with profiling disabled, so the smoke test actually detects a sampler-proxy regression.
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
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.
This pull request introduces several significant changes to the configuration handling and related components in the SolarWinds OpenTelemetry extensions. The main focus is on simplifying and improving the declarative configuration loading process, enhancing test coverage, and refactoring proxy and reporter handling. Notably, it removes the
ReporterProviderclass, updates how configuration is loaded and merged, and refactors proxy configuration parsing to use a shared helper. Additionally, the tests for declarative configuration have been updated to reflect these changes.Configuration Loading and Management:
DeclarativeLoaderto directly extract and merge configuration from thedistributionandsolarwindsnodes, removing theSolarwindsConfigResolverand improving robustness when no configuration is present. The configuration is now initialized viaConfigManager.initialize, and error handling has been simplified. [1] [2]DeclarativeLoaderTestto match the new configuration logic. Tests now check for logging level configuration instead of service key/collector, ensure the loader does not throw on empty models, and verify that configuration merges with previously set values. [1] [2] [3]Proxy Configuration Refactoring:
HttpSamplerComponentProviderandProfilingSpanProcessorComponentProviderwith the sharedProxyHelper.toProxyConfigmethod, ensuring consistent and reusable proxy configuration handling. [1] [2] [3] [4] [5] [6] [7] [8]Reporter and Shutdown Handling:
ReporterProviderclass and its usage, simplifying shutdown logic by only closing client managers and eliminating redundant event reporter handling. [1] [2]Dependency and Import Updates:
libs:loggingas a test dependency in the Gradle build file.These changes collectively modernize and streamline configuration management, improve code clarity, and enhance test reliability.
Test services data