Skip to content

Commit

Permalink
remove global configuration resolver (#1118)
Browse files Browse the repository at this point in the history
Exposing it through Globals can cause a race condition. If something (eg, a contrib module)
retrieves the configuration resolver from Globals too early during autoloading, then it
causes the rest of the objects from Globals to be initialized. This can happen before all
required modules have been registered (depending on autoload->files execution order, which
is variable). Instead, create a simple API config resolver which does the basics of what the
SDK one does (without all the type checking and validation).
  • Loading branch information
brettmc authored Sep 27, 2023
1 parent 3032304 commit 8809c2a
Show file tree
Hide file tree
Showing 12 changed files with 205 additions and 138 deletions.
13 changes: 2 additions & 11 deletions src/API/Behavior/Internal/LogWriterFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use OpenTelemetry\API\Behavior\Internal\LogWriter\NoopLogWriter;
use OpenTelemetry\API\Behavior\Internal\LogWriter\Psr3LogWriter;
use OpenTelemetry\API\Behavior\Internal\LogWriter\StreamLogWriter;
use OpenTelemetry\API\Globals;
use OpenTelemetry\API\Instrumentation\ConfigurationResolver;
use OpenTelemetry\API\LoggerHolder;

class LogWriterFactory
Expand All @@ -18,16 +18,7 @@ class LogWriterFactory

public function create(): LogWriterInterface
{
$dest = Globals::configurationResolver()->getEnum(self::OTEL_PHP_LOG_DESTINATION);
//we might not have an SDK, so attempt to get from environment
if (!$dest) {
$dest = array_key_exists(self::OTEL_PHP_LOG_DESTINATION, $_SERVER)
? $_SERVER[self::OTEL_PHP_LOG_DESTINATION]
: getenv(self::OTEL_PHP_LOG_DESTINATION);
}
if (!$dest) {
$dest = ini_get(self::OTEL_PHP_LOG_DESTINATION);
}
$dest = (new ConfigurationResolver())->getString(self::OTEL_PHP_LOG_DESTINATION);
$logger = LoggerHolder::get();

switch ($dest) {
Expand Down
16 changes: 3 additions & 13 deletions src/API/Globals.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
use function assert;
use Closure;
use const E_USER_WARNING;
use OpenTelemetry\API\Instrumentation\ConfigurationResolverInterface;
use OpenTelemetry\API\Instrumentation\Configurator;
use OpenTelemetry\API\Instrumentation\ContextKeys;
use OpenTelemetry\API\Logs\LoggerProviderInterface;
Expand All @@ -32,20 +31,17 @@ final class Globals
private MeterProviderInterface $meterProvider;
private TextMapPropagatorInterface $propagator;
private LoggerProviderInterface $loggerProvider;
private ConfigurationResolverInterface $configurationResolver;

public function __construct(
TracerProviderInterface $tracerProvider,
MeterProviderInterface $meterProvider,
LoggerProviderInterface $loggerProvider,
TextMapPropagatorInterface $propagator,
ConfigurationResolverInterface $configurationResolver
TextMapPropagatorInterface $propagator
) {
$this->tracerProvider = $tracerProvider;
$this->meterProvider = $meterProvider;
$this->loggerProvider = $loggerProvider;
$this->propagator = $propagator;
$this->configurationResolver = $configurationResolver;
}

public static function tracerProvider(): TracerProviderInterface
Expand All @@ -68,11 +64,6 @@ public static function loggerProvider(): LoggerProviderInterface
return Context::getCurrent()->get(ContextKeys::loggerProvider()) ?? self::globals()->loggerProvider;
}

public static function configurationResolver(): ConfigurationResolverInterface
{
return Context::getCurrent()->get(ContextKeys::configurationResolver()) ?? self::globals()->configurationResolver;
}

/**
* @param Closure(Configurator): Configurator $initializer
*
Expand Down Expand Up @@ -113,11 +104,10 @@ private static function globals(): self
$meterProvider = $context->get(ContextKeys::meterProvider());
$propagator = $context->get(ContextKeys::propagator());
$loggerProvider = $context->get(ContextKeys::loggerProvider());
$configurationResolver = $context->get(ContextKeys::configurationResolver());

assert(isset($tracerProvider, $meterProvider, $loggerProvider, $propagator, $configurationResolver));
assert(isset($tracerProvider, $meterProvider, $loggerProvider, $propagator));

return self::$globals = new self($tracerProvider, $meterProvider, $loggerProvider, $propagator, $configurationResolver);
return self::$globals = new self($tracerProvider, $meterProvider, $loggerProvider, $propagator);
}

/**
Expand Down
77 changes: 77 additions & 0 deletions src/API/Instrumentation/ConfigurationResolver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

declare(strict_types=1);

namespace OpenTelemetry\API\Instrumentation;

class ConfigurationResolver implements ConfigurationResolverInterface
{
public function has(string $name): bool
{
return $this->getVariable($name) !== null;
}

public function getString(string $name): ?string
{
return $this->getVariable($name);
}

public function getBoolean(string $name): ?bool
{
$value = $this->getVariable($name);
if ($value === null) {
return null;
}

return ($value === 'true');
}

public function getInt(string $name): ?int
{
$value = $this->getVariable($name);
if ($value === null) {
return null;
}
if (filter_var($value, FILTER_VALIDATE_INT) === false) {
//log warning
return null;
}

return (int) $value;
}

public function getList(string $name): array
{
$value = $this->getVariable($name);
if ($value === null) {
return [];
}

return explode(',', $value);
}

private function getVariable(string $name): ?string
{
$value = $_SERVER[$name] ?? null;
if ($value !== false && !self::isEmpty($value)) {
assert(is_string($value));

return $value;
}
$value = getenv($name);
if ($value !== false && !self::isEmpty($value)) {
return $value;
}
$value = ini_get($name);
if ($value !== false && !self::isEmpty($value)) {
return $value;
}

return null;
}

private static function isEmpty($value): bool
{
return $value === false || $value === null || $value === '';
}
}
1 change: 0 additions & 1 deletion src/API/Instrumentation/ConfigurationResolverInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,4 @@ public function getString(string $name): ?string;
public function getBoolean(string $name): ?bool;
public function getInt(string $name): ?int;
public function getList(string $name): array;
public function getEnum(string $name): ?string;
}
13 changes: 0 additions & 13 deletions src/API/Instrumentation/Configurator.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ final class Configurator implements ImplicitContextKeyedInterface
private ?MeterProviderInterface $meterProvider = null;
private ?TextMapPropagatorInterface $propagator = null;
private ?LoggerProviderInterface $loggerProvider = null;
private ?ConfigurationResolverInterface $configurationResolver = null;

private function __construct()
{
Expand All @@ -52,7 +51,6 @@ public static function createNoop(): Configurator
->withMeterProvider(new NoopMeterProvider())
->withPropagator(new NoopTextMapPropagator())
->withLoggerProvider(new NoopLoggerProvider())
->withConfigurationResolver(new NoopConfigurationResolver())
;
}

Expand All @@ -77,9 +75,6 @@ public function storeInContext(?ContextInterface $context = null): ContextInterf
if ($this->loggerProvider !== null) {
$context = $context->with(ContextKeys::loggerProvider(), $this->loggerProvider);
}
if ($this->configurationResolver !== null) {
$context = $context->with(ContextKeys::configurationResolver(), $this->configurationResolver);
}

return $context;
}
Expand Down Expand Up @@ -115,12 +110,4 @@ public function withLoggerProvider(?LoggerProviderInterface $loggerProvider): Co

return $self;
}

public function withConfigurationResolver(?ConfigurationResolverInterface $configurationResolver): Configurator
{
$self = clone $this;
$self->configurationResolver = $configurationResolver;

return $self;
}
}
10 changes: 0 additions & 10 deletions src/API/Instrumentation/ContextKeys.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,4 @@ public static function loggerProvider(): ContextKeyInterface

return $instance ??= Context::createKey(LoggerProviderInterface::class);
}

/**
* @return ContextKeyInterface<ConfigurationResolverInterface>
*/
public static function configurationResolver(): ContextKeyInterface
{
static $instance;

return $instance ??= Context::createKey(ConfigurationResolverInterface::class);
}
}
38 changes: 0 additions & 38 deletions src/API/Instrumentation/NoopConfigurationResolver.php

This file was deleted.

40 changes: 0 additions & 40 deletions src/SDK/Common/Configuration/ConfigurationResolver.php

This file was deleted.

2 changes: 0 additions & 2 deletions src/SDK/SdkAutoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
use OpenTelemetry\API\Globals;
use OpenTelemetry\API\Instrumentation\Configurator;
use OpenTelemetry\SDK\Common\Configuration\Configuration;
use OpenTelemetry\SDK\Common\Configuration\ConfigurationResolver;
use OpenTelemetry\SDK\Common\Configuration\Variables;
use OpenTelemetry\SDK\Common\Util\ShutdownHandler;
use OpenTelemetry\SDK\Logs\LoggerProviderFactory;
Expand Down Expand Up @@ -61,7 +60,6 @@ public static function autoload(): bool
->withMeterProvider($meterProvider)
->withLoggerProvider($loggerProvider)
->withPropagator($propagator)
->withConfigurationResolver(new ConfigurationResolver())
;
});

Expand Down
Loading

0 comments on commit 8809c2a

Please sign in to comment.