From 5a395e09da0a2b55e1a400f80b99007449cd69f4 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Thu, 25 Apr 2024 16:21:00 +1000 Subject: [PATCH] enable timeout for http transports (#1275) * enable timeout for http transports Attempt to do our own discovery for some well-known PSR-18 clients, which allows for configuring timeout (and in future certificates, keys etc). This is not complete and a prototype for feedback, but I've updated an example to show that it works for Guzzle and Symfony http clients * improve client discovery * refactor, test, add more implementations * add timeout for logs and metrics --- examples/metrics/exporters/otlp_http.php | 2 +- examples/sdk_builder.php | 2 +- .../exporters/otlp_http_from_factory.php | 1 + examples/traces/exporters/zipkin.php | 2 +- .../traces/features/parent_span_example.php | 2 +- .../troubleshooting/logging_of_span_data.php | 2 +- src/Contrib/Otlp/LogsExporterFactory.php | 11 +++ src/Contrib/Otlp/MetricExporterFactory.php | 12 ++- src/Contrib/Otlp/OtlpHttpTransportFactory.php | 3 +- src/Contrib/Otlp/SpanExporterFactory.php | 14 +++- src/Contrib/Zipkin/SpanExporterFactory.php | 3 +- src/SDK/Common/Configuration/Defaults.php | 10 +-- .../Export/Http/PsrTransportFactory.php | 16 +++- src/SDK/Common/Http/Psr/Client/Discovery.php | 73 +++++++++++++++++++ .../Common/Http/Psr/Client/Discovery/Buzz.php | 28 +++++++ .../Http/Psr/Client/Discovery/CurlClient.php | 33 +++++++++ .../Client/Discovery/DiscoveryInterface.php | 13 ++++ .../Http/Psr/Client/Discovery/Guzzle.php | 21 ++++++ .../Http/Psr/Client/Discovery/Symfony.php | 28 +++++++ tests/TraceContext/W3CTestService/index.php | 2 +- .../Discovery/AbstractDiscoveryTestCase.php | 31 ++++++++ .../Http/Psr/Client/Discovery/BuzzTest.php | 19 +++++ .../Psr/Client/Discovery/CurlClientTest.php | 19 +++++ .../Http/Psr/Client/Discovery/GuzzleTest.php | 19 +++++ .../Http/Psr/Client/Discovery/SymfonyTest.php | 19 +++++ .../Common/Http/Psr/Client/DiscoveryTest.php | 51 +++++++++++++ 26 files changed, 417 insertions(+), 19 deletions(-) create mode 100644 src/SDK/Common/Http/Psr/Client/Discovery.php create mode 100644 src/SDK/Common/Http/Psr/Client/Discovery/Buzz.php create mode 100644 src/SDK/Common/Http/Psr/Client/Discovery/CurlClient.php create mode 100644 src/SDK/Common/Http/Psr/Client/Discovery/DiscoveryInterface.php create mode 100644 src/SDK/Common/Http/Psr/Client/Discovery/Guzzle.php create mode 100644 src/SDK/Common/Http/Psr/Client/Discovery/Symfony.php create mode 100644 tests/Unit/SDK/Common/Http/Psr/Client/Discovery/AbstractDiscoveryTestCase.php create mode 100644 tests/Unit/SDK/Common/Http/Psr/Client/Discovery/BuzzTest.php create mode 100644 tests/Unit/SDK/Common/Http/Psr/Client/Discovery/CurlClientTest.php create mode 100644 tests/Unit/SDK/Common/Http/Psr/Client/Discovery/GuzzleTest.php create mode 100644 tests/Unit/SDK/Common/Http/Psr/Client/Discovery/SymfonyTest.php create mode 100644 tests/Unit/SDK/Common/Http/Psr/Client/DiscoveryTest.php diff --git a/examples/metrics/exporters/otlp_http.php b/examples/metrics/exporters/otlp_http.php index 5f5dc9691..618a6abfc 100644 --- a/examples/metrics/exporters/otlp_http.php +++ b/examples/metrics/exporters/otlp_http.php @@ -14,7 +14,7 @@ $clock = ClockFactory::getDefault(); $reader = new ExportingReader( new MetricExporter( - PsrTransportFactory::discover()->create('http://collector:4318/v1/metrics', \OpenTelemetry\Contrib\Otlp\ContentTypes::JSON) + (new PsrTransportFactory())->create('http://collector:4318/v1/metrics', \OpenTelemetry\Contrib\Otlp\ContentTypes::JSON) ) ); diff --git a/examples/sdk_builder.php b/examples/sdk_builder.php index 0cefc9088..554cfa2bb 100644 --- a/examples/sdk_builder.php +++ b/examples/sdk_builder.php @@ -28,7 +28,7 @@ $reader = new ExportingReader( new MetricExporter( - PsrTransportFactory::discover()->create('http://collector:4318/v1/metrics', 'application/x-protobuf') + (new PsrTransportFactory())->create('http://collector:4318/v1/metrics', 'application/x-protobuf') ) ); diff --git a/examples/traces/exporters/otlp_http_from_factory.php b/examples/traces/exporters/otlp_http_from_factory.php index d836159c5..98fba1b58 100644 --- a/examples/traces/exporters/otlp_http_from_factory.php +++ b/examples/traces/exporters/otlp_http_from_factory.php @@ -11,6 +11,7 @@ */ putenv('OTEL_EXPORTER_OTLP_ENDPOINT=http://collector:4318'); putenv('OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf'); +putenv('OTEL_EXPORTER_OTLP_TIMEOUT=1500'); //1.5s $factory = new \OpenTelemetry\SDK\Trace\TracerProviderFactory(); $tracerProvider = $factory->create(); diff --git a/examples/traces/exporters/zipkin.php b/examples/traces/exporters/zipkin.php index 6be6d2549..4d8f4d2cb 100644 --- a/examples/traces/exporters/zipkin.php +++ b/examples/traces/exporters/zipkin.php @@ -11,7 +11,7 @@ use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor; use OpenTelemetry\SDK\Trace\TracerProvider; -$transport = PsrTransportFactory::discover()->create('http://zipkin:9411/api/v2/spans', 'application/json'); +$transport = (new PsrTransportFactory())->create('http://zipkin:9411/api/v2/spans', 'application/json'); $zipkinExporter = new ZipkinExporter( $transport ); diff --git a/examples/traces/features/parent_span_example.php b/examples/traces/features/parent_span_example.php index 585809b15..5e36da8e3 100644 --- a/examples/traces/features/parent_span_example.php +++ b/examples/traces/features/parent_span_example.php @@ -17,7 +17,7 @@ // zipkin exporter $zipkinExporter = new ZipkinExporter( - PsrTransportFactory::discover()->create('http://zipkin:9411/api/v2/spans', 'application/json') + (new PsrTransportFactory())->create('http://zipkin:9411/api/v2/spans', 'application/json') ); $tracerProvider = new TracerProvider( diff --git a/examples/troubleshooting/logging_of_span_data.php b/examples/troubleshooting/logging_of_span_data.php index ac0cdfa78..a1e42b8e9 100644 --- a/examples/troubleshooting/logging_of_span_data.php +++ b/examples/troubleshooting/logging_of_span_data.php @@ -44,7 +44,7 @@ */ //$exporterEndpoint = 'http://example.com:9411/api/v2/spans'; $exporter = new ZipkinExporter( - PsrTransportFactory::discover()->create($exporterEndpoint, 'application/json'), + (new PsrTransportFactory())->create($exporterEndpoint, 'application/json'), ); /** * Decorate the Exporter diff --git a/src/Contrib/Otlp/LogsExporterFactory.php b/src/Contrib/Otlp/LogsExporterFactory.php index c70b1649d..a92cffa3b 100644 --- a/src/Contrib/Otlp/LogsExporterFactory.php +++ b/src/Contrib/Otlp/LogsExporterFactory.php @@ -43,6 +43,7 @@ private function buildTransport(string $protocol): TransportInterface $headers = OtlpUtil::getHeaders(Signals::LOGS); $compression = $this->getCompression(); + $timeout = $this->getTimeout(); $factoryClass = Registry::transportFactory($protocol); $factory = $this->transportFactory ?: new $factoryClass(); @@ -52,6 +53,7 @@ private function buildTransport(string $protocol): TransportInterface Protocols::contentType($protocol), $headers, $compression, + $timeout, ); } @@ -76,4 +78,13 @@ private function getEndpoint(string $protocol): string return HttpEndpointResolver::create()->resolveToString($endpoint, Signals::LOGS); } + + private function getTimeout(): float + { + $value = Configuration::has(Variables::OTEL_EXPORTER_OTLP_LOGS_TIMEOUT) ? + Configuration::getInt(Variables::OTEL_EXPORTER_OTLP_LOGS_TIMEOUT) : + Configuration::getInt(Variables::OTEL_EXPORTER_OTLP_TIMEOUT); + + return $value/1000; + } } diff --git a/src/Contrib/Otlp/MetricExporterFactory.php b/src/Contrib/Otlp/MetricExporterFactory.php index d59ecdc64..1ba7488f3 100644 --- a/src/Contrib/Otlp/MetricExporterFactory.php +++ b/src/Contrib/Otlp/MetricExporterFactory.php @@ -44,12 +44,12 @@ private function buildTransport(string $protocol): TransportInterface /** * @todo (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#periodic-exporting-metricreader) * - OTEL_METRIC_EXPORT_INTERVAL - * - OTEL_METRIC_EXPORT_TIMEOUT */ $endpoint = $this->getEndpoint($protocol); $headers = OtlpUtil::getHeaders(Signals::METRICS); $compression = $this->getCompression(); + $timeout = $this->getTimeout(); $factoryClass = Registry::transportFactory($protocol); $factory = $this->transportFactory ?: new $factoryClass(); @@ -59,6 +59,7 @@ private function buildTransport(string $protocol): TransportInterface Protocols::contentType($protocol), $headers, $compression, + $timeout, ); } @@ -84,6 +85,15 @@ private function getCompression(): string Configuration::getEnum(Variables::OTEL_EXPORTER_OTLP_COMPRESSION, self::DEFAULT_COMPRESSION); } + private function getTimeout(): float + { + $value = Configuration::has(Variables::OTEL_EXPORTER_OTLP_METRICS_TIMEOUT) ? + Configuration::getInt(Variables::OTEL_EXPORTER_OTLP_METRICS_TIMEOUT) : + Configuration::getInt(Variables::OTEL_EXPORTER_OTLP_TIMEOUT); + + return $value/1000; + } + private function getEndpoint(string $protocol): string { if (Configuration::has(Variables::OTEL_EXPORTER_OTLP_METRICS_ENDPOINT)) { diff --git a/src/Contrib/Otlp/OtlpHttpTransportFactory.php b/src/Contrib/Otlp/OtlpHttpTransportFactory.php index 637e3ae5f..f40af681a 100644 --- a/src/Contrib/Otlp/OtlpHttpTransportFactory.php +++ b/src/Contrib/Otlp/OtlpHttpTransportFactory.php @@ -28,6 +28,7 @@ public function create( $compression = null; } - return PsrTransportFactory::discover()->create($endpoint, $contentType, $headers, $compression, $timeout, $retryDelay, $maxRetries, $cacert, $cert, $key); + return (new PsrTransportFactory()) + ->create($endpoint, $contentType, $headers, $compression, $timeout, $retryDelay, $maxRetries, $cacert, $cert, $key); } } diff --git a/src/Contrib/Otlp/SpanExporterFactory.php b/src/Contrib/Otlp/SpanExporterFactory.php index 67627a7b9..1ef7d49aa 100644 --- a/src/Contrib/Otlp/SpanExporterFactory.php +++ b/src/Contrib/Otlp/SpanExporterFactory.php @@ -21,7 +21,7 @@ class SpanExporterFactory implements SpanExporterFactoryInterface private const DEFAULT_COMPRESSION = 'none'; - public function __construct(private ?TransportFactoryInterface $transportFactory = null) + public function __construct(private readonly ?TransportFactoryInterface $transportFactory = null) { } @@ -46,11 +46,12 @@ private function buildTransport(): TransportInterface $endpoint = $this->getEndpoint($protocol); $headers = OtlpUtil::getHeaders(Signals::TRACE); $compression = $this->getCompression(); + $timeout = $this->getTimeout(); $factoryClass = Registry::transportFactory($protocol); $factory = $this->transportFactory ?: new $factoryClass(); - return $factory->create($endpoint, $contentType, $headers, $compression); + return $factory->create($endpoint, $contentType, $headers, $compression, $timeout); } private function getProtocol(): string @@ -81,4 +82,13 @@ private function getCompression(): string Configuration::getEnum(Variables::OTEL_EXPORTER_OTLP_TRACES_COMPRESSION) : Configuration::getEnum(Variables::OTEL_EXPORTER_OTLP_COMPRESSION, self::DEFAULT_COMPRESSION); } + + private function getTimeout(): float + { + $value = Configuration::has(Variables::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT) ? + Configuration::getInt(Variables::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT) : + Configuration::getInt(Variables::OTEL_EXPORTER_OTLP_TIMEOUT); + + return $value/1000; + } } diff --git a/src/Contrib/Zipkin/SpanExporterFactory.php b/src/Contrib/Zipkin/SpanExporterFactory.php index fc352a633..0714d48da 100644 --- a/src/Contrib/Zipkin/SpanExporterFactory.php +++ b/src/Contrib/Zipkin/SpanExporterFactory.php @@ -15,7 +15,8 @@ class SpanExporterFactory implements SpanExporterFactoryInterface public function create(): SpanExporterInterface { $endpoint = Configuration::getString(Variables::OTEL_EXPORTER_ZIPKIN_ENDPOINT); - $transport = PsrTransportFactory::discover()->create($endpoint, 'application/json'); + $timeout = Configuration::getInt(Variables::OTEL_EXPORTER_ZIPKIN_TIMEOUT)/1000; + $transport = (new PsrTransportFactory())->create(endpoint: $endpoint, contentType: 'application/json', timeout: $timeout); return new Exporter($transport); } diff --git a/src/SDK/Common/Configuration/Defaults.php b/src/SDK/Common/Configuration/Defaults.php index 7228270a6..fcfea6e4e 100644 --- a/src/SDK/Common/Configuration/Defaults.php +++ b/src/SDK/Common/Configuration/Defaults.php @@ -70,10 +70,10 @@ interface Defaults public const OTEL_EXPORTER_OTLP_METRICS_INSECURE = 'false'; public const OTEL_EXPORTER_OTLP_LOGS_INSECURE = 'false'; // Timeout (seconds) - public const OTEL_EXPORTER_OTLP_TIMEOUT = 10; - public const OTEL_EXPORTER_OTLP_TRACES_TIMEOUT = 10; - public const OTEL_EXPORTER_OTLP_METRICS_TIMEOUT = 10; - public const OTEL_EXPORTER_OTLP_LOGS_TIMEOUT = 10; + public const OTEL_EXPORTER_OTLP_TIMEOUT = 10000; //10s + public const OTEL_EXPORTER_OTLP_TRACES_TIMEOUT = 10000; + public const OTEL_EXPORTER_OTLP_METRICS_TIMEOUT = 10000; + public const OTEL_EXPORTER_OTLP_LOGS_TIMEOUT = 10000; // Protocol public const OTEL_EXPORTER_OTLP_PROTOCOL = 'http/protobuf'; public const OTEL_EXPORTER_OTLP_TRACES_PROTOCOL = 'http/protobuf'; @@ -85,7 +85,7 @@ interface Defaults */ public const OTEL_EXPORTER_ZIPKIN_ENDPOINT = 'http://localhost:9411/api/v2/spans'; // Timeout (seconds) - public const OTEL_EXPORTER_ZIPKIN_TIMEOUT = 10; + public const OTEL_EXPORTER_ZIPKIN_TIMEOUT = 10000; //10s /** * Prometheus Exporter * @see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#prometheus-exporter diff --git a/src/SDK/Common/Export/Http/PsrTransportFactory.php b/src/SDK/Common/Export/Http/PsrTransportFactory.php index 1249d1eac..433ef7c31 100644 --- a/src/SDK/Common/Export/Http/PsrTransportFactory.php +++ b/src/SDK/Common/Export/Http/PsrTransportFactory.php @@ -10,6 +10,7 @@ use Http\Discovery\Psr18ClientDiscovery; use InvalidArgumentException; use OpenTelemetry\SDK\Common\Export\TransportFactoryInterface; +use OpenTelemetry\SDK\Common\Http\Psr\Client\Discovery; use Psr\Http\Client\ClientInterface; use Psr\Http\Message\RequestFactoryInterface; use Psr\Http\Message\StreamFactoryInterface; @@ -17,9 +18,9 @@ final class PsrTransportFactory implements TransportFactoryInterface { public function __construct( - private readonly ClientInterface $client, - private readonly RequestFactoryInterface $requestFactory, - private readonly StreamFactoryInterface $streamFactory, + private ?ClientInterface $client = null, + private ?RequestFactoryInterface $requestFactory = null, + private ?StreamFactoryInterface $streamFactory = null, ) { } @@ -43,6 +44,12 @@ public function create( } assert(!empty($endpoint)); + $this->client ??= Discovery::find([ + 'timeout' => $timeout, + ]); + $this->requestFactory ??= Psr17FactoryDiscovery::findRequestFactory(); + $this->streamFactory ??= Psr17FactoryDiscovery::findStreamFactory(); + return new PsrTransport( $this->client, $this->requestFactory, @@ -56,6 +63,9 @@ public function create( ); } + /** + * @deprecated + */ public static function discover(): self { return new self( diff --git a/src/SDK/Common/Http/Psr/Client/Discovery.php b/src/SDK/Common/Http/Psr/Client/Discovery.php new file mode 100644 index 000000000..a4ac84e5d --- /dev/null +++ b/src/SDK/Common/Http/Psr/Client/Discovery.php @@ -0,0 +1,73 @@ +> + */ + private const DEFAULTS = [ + Guzzle::class, + Symfony::class, + Buzz::class, + CurlClient::class, + ]; + + /** + * Attempt discovery of a configurable psr-18 http client, falling back to Psr18ClientDiscovery. + */ + public static function find(array $options = []): ClientInterface + { + $options = array_filter($options); + + foreach (self::discoverers() as $clientDiscovery) { + /** @var DiscoveryInterface $clientDiscovery */ + if ($clientDiscovery->available()) { + return $clientDiscovery->create($options); + } + } + + return Psr18ClientDiscovery::find(); + } + + /** + * @internal + */ + public static function setDiscoverers(array $discoverers): void + { + self::$discoverers = $discoverers; + } + + /** + * @internal + */ + public static function reset(): void + { + self::$discoverers = null; + } + + private static function discoverers(): Generator + { + foreach (self::$discoverers ?? self::DEFAULTS as $discoverer) { + if (is_string($discoverer)) { + yield new $discoverer(); + } else { + yield $discoverer; + } + } + } +} diff --git a/src/SDK/Common/Http/Psr/Client/Discovery/Buzz.php b/src/SDK/Common/Http/Psr/Client/Discovery/Buzz.php new file mode 100644 index 000000000..14733e859 --- /dev/null +++ b/src/SDK/Common/Http/Psr/Client/Discovery/Buzz.php @@ -0,0 +1,28 @@ + $options['timeout'] ?? null, + ]; + + /** @phpstan-ignore-next-line */ + return new Client(options: array_filter($options)); + } +} diff --git a/src/SDK/Common/Http/Psr/Client/Discovery/DiscoveryInterface.php b/src/SDK/Common/Http/Psr/Client/Discovery/DiscoveryInterface.php new file mode 100644 index 000000000..dfe5036a4 --- /dev/null +++ b/src/SDK/Common/Http/Psr/Client/Discovery/DiscoveryInterface.php @@ -0,0 +1,13 @@ +create('http://zipkin:9412/api/v2/spans', 'application/json') + (new PsrTransportFactory())->create('http://zipkin:9412/api/v2/spans', 'application/json') ), ClockFactory::getDefault() ), diff --git a/tests/Unit/SDK/Common/Http/Psr/Client/Discovery/AbstractDiscoveryTestCase.php b/tests/Unit/SDK/Common/Http/Psr/Client/Discovery/AbstractDiscoveryTestCase.php new file mode 100644 index 000000000..9435b70ff --- /dev/null +++ b/tests/Unit/SDK/Common/Http/Psr/Client/Discovery/AbstractDiscoveryTestCase.php @@ -0,0 +1,31 @@ +expectNotToPerformAssertions(); + $this->getInstance()->available(); + } + + public function test_create(): void + { + try { + $client = $this->getInstance()->create(['timeout' => 1234]); + $this->assertInstanceOf(ClientInterface::class, $client); + } catch (\Error) { + //dependencies may not be installed in test environment + $this->expectNotToPerformAssertions(); + } + } +} diff --git a/tests/Unit/SDK/Common/Http/Psr/Client/Discovery/BuzzTest.php b/tests/Unit/SDK/Common/Http/Psr/Client/Discovery/BuzzTest.php new file mode 100644 index 000000000..1ea9b2662 --- /dev/null +++ b/tests/Unit/SDK/Common/Http/Psr/Client/Discovery/BuzzTest.php @@ -0,0 +1,19 @@ +assertInstanceOf(ClientInterface::class, $client); + } + + public function test_discover(): void + { + $timeout = 31415; + $one = $this->createMock(DiscoveryInterface::class); + $one->expects($this->once())->method('available')->willReturn(false); + $one->expects($this->never())->method('create'); + $two = $this->createMock(DiscoveryInterface::class); + $two->expects($this->once())->method('available')->willReturn(true); + $two->expects($this->once()) + ->method('create') + ->with($this->equalTo(['timeout' => $timeout])) + ->willReturn($this->createMock(ClientInterface::class)); + + Discovery::setDiscoverers([$one, $two]); + Discovery::find(['timeout' => 31415]); + } + + public function test_fallback(): void + { + Discovery::setDiscoverers([]); + $this->assertInstanceOf(ClientInterface::class, Discovery::find()); + } +}