split config into own package

[brettmc]

Moving configuration code out of the SDK means that it can be used by components that should not depend on the SDK.

• move config into own package
• update all core components which were self-managing config to use the config package
• add some tests for config package to get coverage to 100%
• add missing type-hints to config package
• add missing config variables (OTEL_PHP_DEBUG_SCOPES_DISABLED and OTEL_PHP_FIBERS_ENABLED)

Prior to merge:

• [ ] create gitsplit target repo
• [ ] document that some components which formerly allowed truthy/falsey values will (or soon will) only accept true or false

Closes: #1245

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 72.83%. Comparing base (f2cba82) to head (28fe251).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1248      +/-   ##
============================================
- Coverage     73.97%   72.83%   -1.14%     
+ Complexity     2365     2335      -30     
============================================
  Files           347      346       -1     
  Lines          7071     6988      -83     
============================================
- Hits           5231     5090     -141     
- Misses         1840     1898      +58     

Flag	Coverage Δ
8.1	?
8.2	?
8.3	?
8.4	72.83% <83.87%> (-0.01%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/API/Behavior/Internal/LogWriterFactory.php	71.42% <100.00%> (-7.52%)	:arrow_down:
...g/Configuration/Accessor/ClassConstantAccessor.php	100.00% <100.00%> (ø)
...c/Config/Configuration/Accessor/PhpIniAccessor.php	100.00% <100.00%> (ø)
src/Config/Configuration/Configuration.php	100.00% <100.00%> (ø)
src/Config/Configuration/Parser/BooleanParser.php	100.00% <100.00%> (ø)
src/Config/Configuration/Parser/ListParser.php	100.00% <100.00%> (ø)
src/Config/Configuration/Parser/MapParser.php	100.00% <100.00%> (ø)
src/Config/Configuration/Parser/RatioParser.php	100.00% <100.00%> (ø)
...onfig/Configuration/Resolver/CompositeResolver.php	100.00% <100.00%> (ø)
...fig/Configuration/Resolver/EnvironmentResolver.php	100.00% <100.00%> (ø)
... and 24 more

... and 2 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f2cba82...28fe251. Read the comment docs.

[Nevay]

IMO we shouldn't move everything out of the SDK, I would keep Variables (and Defaults + KnownValues if needed) in the component that uses them.

Configuration::validateVariableType() + ValueTypes + VariableTypes can be removed. They are only used to verify that code calls the correct method; no need to check this during runtime / should be verified by tests.

Because all of our configuration-management code is in the SDK, it's not usable by contrib modules etc which need to deal with config

We should also take file configuration and configuration via code into account. The configuration formats do not match 1:1 -> an intermediate instrumentation configuration model would make sense instead of using Configuration::getXXX() directly. This would be especially useful for complex configuration options that cannot be expressed as environment variable, e.g. configuration for a SQL sanitizer.

Example: HTTP instrumentation needs to know which request and response headers should be recorded as attributes. Env/ini configuration currently uses otel.instrumentation.http.request_headers and otel.instrumentation.http.response_headers, while file configuration might use:

instrumentation:
  config:
  - http_client:
      request_headers: []
      response_headers: []

Both formats could then populate the following intermediate model that is made available via a registry InstrumentationConfigurationRegistry::get(HttpClientInstrumentation::class).

class HttpClientInstrumentation implements InstrumentationConfiguration {
    public function __construct(
        public readonly array $requestHeaders = [],
        public readonly array $responseHeaders = [],
    ) {}
}

[brettmc]

@Nevay

Configuration::validateVariableType() + ValueTypes + VariableTypes can be removed

Removed.

IMO we shouldn't move everything out of the SDK, I would keep Variables (and Defaults + KnownValues if needed) in the component that uses them.

I'm hesitant to do this, because then contrib packages need to depend on SDK to fetch config (unless we start splitting config up into SDK, general, and possibly also for any packages that have custom config)?

Both formats could then populate the following intermediate model that is made available via a registry

I started playing around with this. I thought perhaps factories could accept an InstrumentationConfiguration. It felt like double-handling if a ComponentProvider created an intermediate configuration, and as it stands the various factories build their own config as they go. Maybe this is worth a follow-up PR?

[Nevay]

I'm hesitant to do this, because then contrib packages need to depend on SDK to fetch config (unless we start splitting config up into SDK, general, and possibly also for any packages that have custom config)?

Variables & co. contain only constants, not the code to actually fetch config.

I started playing around with this. I thought perhaps factories could accept an InstrumentationConfiguration. It felt like double-handling if a ComponentProvider created an intermediate configuration, and as it stands the various factories build their own config as they go. Maybe this is worth a follow-up PR?

I've created a prototype that utilizes the idea of https://github.com/open-telemetry/opentelemetry-php/pull/1274 to inject configuration into instrumentation: https://github.com/Nevay/opentelemetry-php/commit/2ecf4b22c0c8d76f07d6238cd75c27167d396b15

[brettmc]

Closing, replaced by #1304