PoC: Configuration\CompositeResolver and SPI discovery #1523
Conversation
… resolvers via SPI.
| use OpenTelemetry\SDK\Common\Configuration\Resolver\ResolverInterface; | ||
| use function str_starts_with; | ||
|
|
||
| #[PackageDependency('vlucas/phpdotenv', '^5.0')] |
There was a problem hiding this comment.
There was a problem hiding this comment.
No, I was only concerned about implementing our own .env parsing.
| $env ??= array_filter( | ||
| Dotenv::createArrayBacked([getcwd()])->safeLoad(), | ||
| // Discard any environment variables that do not start with OTEL_ | ||
| fn (string $name) => str_starts_with($name, 'OTEL_'), |
There was a problem hiding this comment.
Assuming that the same approach will be used for file configuration EnvReader:
IMO we shouldn't remove non-OTEL_ keys, users might want to reference them in their config file.
There was a problem hiding this comment.
Great point, I shall drop this filtering. 👍
|
|
||
| try { | ||
| $env ??= array_filter( | ||
| Dotenv::createArrayBacked([getcwd()])->safeLoad(), |
There was a problem hiding this comment.
Should use ::createImmutable() as .env might reference environment variables defined outside of the .env file. For example the following should work:
# .env
OTEL_PHP_AUTOLOAD_ENABLED=true
OTEL_LOG_LEVEL=${LOG_LEVEL}
LOG_LEVEL=debug php vendor/autoload.phpIf using ::createImmutable(): should backup $_SERVER and $_ENV before reading the .env file to avoid visible side effects; I've decided to write an implementation too, see
Nevay/otel-sdk-config@d5be39f#diff-ba9503c0f072a1539d96dc365912b509cc45ce92648a7765c53a86c9419687a7 for reference.
There was a problem hiding this comment.
Thanks for highlighting this, I'm hoping it's okay that I port your improvements over?! 😅
Co-authored-by: Tobias Bachert <[email protected]>
ChrisLightfootWild
force-pushed
the
spi/dotenv
branch
from
6a8e0fc to
93c01c3
Compare
| @@ -60,6 +60,9 @@ | |||
| "spi": { | |||
| "OpenTelemetry\\API\\Instrumentation\\AutoInstrumentation\\HookManagerInterface": [ | |||
| "OpenTelemetry\\API\\Instrumentation\\AutoInstrumentation\\ExtensionHookManager" | |||
| ], | |||
| "OpenTelemetry\\SDK\\Common\\Configuration\\Resolver\\ResolverInterface": [ | |||
| "OpenTelemetry\\SDK\\Common\\Configuration\\Resolver\\SdkConfigurationResolver" | |||
There was a problem hiding this comment.
Only kicks in when open-telemetry/sdk-configuration is available.
| @@ -18,6 +19,7 @@ public static function instance(): self | |||
| { | |||
| static $instance; | |||
| $instance ??= new self([ | |||
| ...ServiceLoader::load(ResolverInterface::class), | |||
There was a problem hiding this comment.
This effectively replaces the below resolvers because it will use the ServerEnvSource & PhpIniEnvSource sources.
--
Should open-telemetry/sdk-configuration be promoted to the SDK rather than remain as an optional dependency? 🤔
ChrisLightfootWild
force-pushed
the
spi/dotenv
branch
from
d001679 to
21e4902
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1523 +/- ##
============================================
- Coverage 70.76% 70.50% -0.26%
- Complexity 2763 2776 +13
============================================
Files 408 412 +4
Lines 8346 8392 +46
============================================
+ Hits 5906 5917 +11
- Misses 2440 2475 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| "bamarni/composer-bin-plugin": "^1.8", | ||
| "dg/bypass-finals": "^1.4", | ||
| "grpc/grpc": "^1.30", |
There was a problem hiding this comment.
Sort package config re-ordered this.
Proof of concept, to cover the use-case where multi-application installs operate on shared-infra and wish to provide their own configuration.
See #1436.
Any suggestions on the best way to approach adding tests for these optional dependencies?