open-telemetry
/
opentelemetry-php
mirror of https://github.com/open-telemetry/opentelemetry-php.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix dependency violations in exporter factory (#525) 

* Relax fromConnectionString requirements

* Remove contrib dependencies from exporter factory

* Add SemConv to dependency check

* Add SemConv as required CI check

* Remove dependency from ZendObserverFiber on SDK

* Fix syntax error

* Fix style

* Cast scheme to string to avoid nullable argument

* Make up for removing false positive code coverage.

* Fix CS

* Fix assertions

* Remove unused method
This commit is contained in:
Timo Michna 2021-12-29 15:47:47 +01:00 committed by GitHub
parent 645754a104
commit 4ace6c1b66
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 208 additions and 133 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.github/workflows/php.yml vendored
View File

@ -56,6 +56,9 @@ jobs:
- name: Check Style
run: vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.php --dry-run --stop-on-violation --using-cache=no -vvv
- name: Check Dependencies
run: vendor/bin/deptrac --formatter=github-actions
- name: Run Phan
env:
PHAN_DISABLE_XDEBUG_WARN: 1

2
Makefile
View File

@ -1,7 +1,7 @@
PHP_VERSION ?= 7.4
DC_RUN_PHP = docker-compose run --rm php
all: update style phan psalm phpstan test
all: update style deptrac phan psalm phpstan test
install:
$(DC_RUN_PHP) env XDEBUG_MODE=off composer install
update:

9
depfile.yaml
View File

@ -22,6 +22,12 @@ layers:
-
type: className
regex: OpenTelemetry\\Context\\*
-
name: SemConv
collectors:
-
type: className
regex: OpenTelemetry\\SemConv\\*
-
name: Contrib
collectors:
@ -40,9 +46,12 @@ ruleset:
- SDK
- Context
- Proto
- SemConv
Context: ~
SemConv: ~
SDK:
- API
- Context
- SemConv
API:
- Context

5
src/Context/ZendObserverFiber.php
View File

@ -13,19 +13,16 @@ namespace OpenTelemetry\Context;
use FFI;
use FFI\Exception;
use OpenTelemetry\SDK\EnvironmentVariablesTrait;
class ZendObserverFiber
{
use EnvironmentVariablesTrait;
protected static $fibers = null;
public function isEnabled(): bool
{
return (
PHP_VERSION_ID >= 80100 &&
true === $this->getBooleanFromEnvironment('OTEL_PHP_FIBERS_ENABLED', false) &&
(in_array(getenv('OTEL_PHP_FIBERS_ENABLED'), ['true', 'on', '1'])) &&
class_exists(FFI::class)
);
}

5
src/Contrib/OtlpGrpc/Exporter.php
View File

@ -182,9 +182,8 @@ class Exporter implements SpanExporterInterface
return Grpc\ChannelCredentials::createInsecure();
}
/** @inheritDoc */
public static function fromConnectionString(string $endpointUrl, string $name = null, $args = null)
public static function fromConnectionString(string $endpointUrl = null, string $name = null, $args = null): Exporter
{
return new Exporter($endpointUrl);
return is_string($endpointUrl) ? new Exporter($endpointUrl) : new Exporter();
}
}

8
src/Contrib/OtlpHttp/Exporter.php
View File

@ -157,8 +157,7 @@ class Exporter implements SpanExporterInterface
return (string) $dsn;
}
/** @inheritDoc */
public static function fromConnectionString(string $endpointUrl = null, string $name = null, $args = null)
public static function fromConnectionString(string $endpointUrl = null, string $name = null, $args = null): Exporter
{
return new Exporter(
HttpClientDiscovery::find(),
@ -167,11 +166,6 @@ class Exporter implements SpanExporterInterface
);
}
public static function create(): Exporter
{
return self::fromConnectionString();
}
private function shouldCompress(): bool
{
return $this->compression === 'gzip' && function_exists('gzencode');

172
src/SDK/Trace/ExporterFactory.php
View File

@ -5,81 +5,71 @@ declare(strict_types=1);
namespace OpenTelemetry\SDK\Trace;
use InvalidArgumentException;
use Nyholm\Dsn\Configuration\Dsn;
use Nyholm\Dsn\Configuration\Url;
use Nyholm\Dsn\DsnParser;
use OpenTelemetry\Contrib\Jaeger\Exporter as JaegerExporter;
use OpenTelemetry\Contrib\Newrelic\Exporter as NewrelicExporter;
use OpenTelemetry\Contrib\OtlpGrpc\Exporter as OtlpGrpcExporter;
use OpenTelemetry\Contrib\OtlpHttp\Exporter as OtlpHttpExporter;
use OpenTelemetry\Contrib\Zipkin\Exporter as ZipkinExporter;
use OpenTelemetry\Contrib\ZipkinToNewrelic\Exporter as ZipkinToNewrelicExporter;
use OpenTelemetry\SDK\EnvironmentVariablesTrait;
use OpenTelemetry\SDK\Trace\SpanExporter\ConsoleSpanExporter;
class ExporterFactory
{
use EnvironmentVariablesTrait;
private string $name;
private array $allowedExporters = ['jaeger' => true, 'zipkin' => true, 'newrelic' => true, 'otlp' => true, 'otlpgrpc' => true, 'otlphttp' => true ,'zipkintonewrelic' => true, 'console' => true];
private const KNOWN_EXPORTERS = [
'console' => '\OpenTelemetry\SDK\Trace\SpanExporter\ConsoleSpanExporter',
'logger+file' => '\OpenTelemetry\SDK\Trace\SpanExporter\LoggerExporter',
'jaeger+http' => '\OpenTelemetry\Contrib\Jaeger\Exporter',
'zipkin+http' => '\OpenTelemetry\Contrib\Zipkin\Exporter',
'otlp+grpc' => '\OpenTelemetry\Contrib\OtlpGrpc\Exporter',
'otlp+http' => '\OpenTelemetry\Contrib\OtlpHttp\Exporter',
'newrelic+http' => '\OpenTelemetry\Contrib\Newrelic\Exporter',
'zipkintonewrelic+http' => '\OpenTelemetry\Contrib\ZipkinToNewrelic\Exporter',
// this entry exists only for testing purposes
'test+http' => '\OpenTelemetry\Contrib\Test\Exporter',
];
public function __construct(string $name)
private const DEFAULT_SERVICE_NAME = 'unknown_service';
private string $serviceName;
public function __construct(string $serviceName = self::DEFAULT_SERVICE_NAME)
{
$this->name = $name;
$this->serviceName = $serviceName;
}
/**
* Returns the corresponding Exporter via the configuration string
*
* @param string $configurationString String containing unextracted information for Exporter creation
* Should follow the format: contribType+baseUrl?option1=a
* @param string $connectionString String containing information for Exporter creation
* Should follow the format: type+baseUri?option1=a
* Query string is optional and based on the Exporter
*/
public function fromConnectionString(string $configurationString): SpanExporterInterface
public function fromConnectionString(string $connectionString): SpanExporterInterface
{
$strArr = explode('+', $configurationString);
// checks if input is given with the format type+baseUrl
if (sizeof($strArr) !== 2) {
throw new InvalidArgumentException('Invalid format.');
if (in_array($connectionString, ['console', 'otlp+http'])) {
return self::buildExporter($connectionString);
}
$contribName = strtolower($strArr[0]);
$endpointUrl = $strArr[1];
$dsn = DsnParser::parseUrl($connectionString);
if (!$this->isAllowed($contribName)) {
throw new InvalidArgumentException('Invalid contrib name.');
self::validateScheme((string) $dsn->getScheme());
$endpoint = self::getEndpointFromDsn($dsn);
$serviceName = $this->resolveServiceName($dsn);
if (in_array(self::normalizeScheme((string) $dsn->getScheme()), ['newrelic+http', 'zipkintonewrelic+http'])) {
return self::buildExporter(
(string) $dsn->getScheme(),
$endpoint,
$serviceName,
self::getParameterFromDsn($dsn, 'licenseKey')
);
}
// @phan-suppress-next-line PhanUndeclaredClassMethod
$dsn = empty($endpointUrl) ? '' : DsnParser::parse($endpointUrl);
$endpointUrl = $this->parseBaseUrl($dsn);
// parameters are only retrieved if there was an endpoint given
$args = empty($dsn) ? [] : $dsn->getParameters();
$scheme = empty($dsn) ? '' : $dsn->getScheme();
switch ($contribName) {
case 'jaeger':
return JaegerExporter::fromConnectionString($endpointUrl, $this->name);
case 'zipkin':
return ZipkinExporter::fromConnectionString($endpointUrl, $this->name);
case 'newrelic':
return NewrelicExporter::fromConnectionString($endpointUrl, $this->name, $args['licenseKey'] ?? null);
case 'otlp':
switch ($scheme) {
case 'grpc':
return OtlpGrpcExporter::fromConnectionString($endpointUrl);
case 'http':
return OtlpHttpExporter::fromConnectionString($endpointUrl);
default:
throw new InvalidArgumentException('Invalid otlp scheme');
}
// no break
case 'zipkintonewrelic':
return ZipkinToNewrelicExporter::fromConnectionString($endpointUrl, $this->name, $args['licenseKey'] ?? null);
case 'console':
return ConsoleSpanExporter::fromConnectionString($endpointUrl);
default:
throw new InvalidArgumentException('Invalid contrib name.');
}
return self::buildExporter(
(string) $dsn->getScheme(),
$endpoint,
$serviceName
);
}
public function fromEnvironment(): ?SpanExporterInterface
@ -109,9 +99,9 @@ class ExporterFactory
}
switch ($protocol) {
case 'grpc':
return new OtlpGrpcExporter();
return self::buildExporter('otlp+grpc');
case 'http/protobuf':
return OtlpHttpExporter::create();
return self::buildExporter('otlp+http');
case 'http/json':
throw new InvalidArgumentException('otlp+http/json not implemented');
default:
@ -119,29 +109,73 @@ class ExporterFactory
}
// no break
case 'console':
return new ConsoleSpanExporter();
return self::buildExporter('console');
default:
throw new InvalidArgumentException('Invalid exporter name');
}
}
private function isAllowed(string $exporter)
private function resolveServiceName(Dsn $dsn): string
{
return array_key_exists($exporter, $this->allowedExporters) && $this->allowedExporters[$exporter];
return self::getParameterFromDsn($dsn, 'serviceName') ?? $this->serviceName;
}
// constructs the baseUrl with the arguments retrieved from the raw baseUrl
private function parseBaseUrl($dsn)
private static function getParameterFromDsn(Dsn $dsn, string $parameter): ?string
{
if ($dsn == false) {
throw new InvalidArgumentException('Invalid endpoint');
}
$parsedUrl = '';
$parsedUrl .= empty($dsn->getScheme()) ? '' : $dsn->getScheme() . '://';
$parsedUrl .= empty($dsn->getHost()) ? '' : $dsn->getHost();
$parsedUrl .= empty($dsn->getPort()) ? '' : ':' . $dsn->getPort();
$parsedUrl .= empty($dsn->getPath()) ? '' : $dsn->getPath();
$parameters = $dsn->getParameters();
return $parsedUrl;
foreach ([$parameter, strtolower($parameter)] as $name) {
if (array_key_exists($name, $parameters)) {
return $parameters[$name];
}
}
return null;
}
private static function getEndpointFromDsn(Url $dsn): string
{
return (string) new Url(
self::getProtocolFromScheme((string) $dsn->getScheme()),
$dsn->getHost(),
$dsn->getPort(),
$dsn->getPath(),
[],
$dsn->getAuthentication()
);
}
private static function buildExporter(string $scheme, string $endpoint = null, string $name = null, $args = null): SpanExporterInterface
{
$exporterClass = self::KNOWN_EXPORTERS[self::normalizeScheme($scheme)];
self::validateExporterClass($exporterClass);
return call_user_func([$exporterClass, 'fromConnectionString'], $endpoint, $name, $args);
}
private static function validateScheme(string $scheme)
{
if (!array_key_exists(self::normalizeScheme($scheme), self::KNOWN_EXPORTERS)) {
throw new InvalidArgumentException('Invalid exporter scheme: ' . $scheme);
}
}
private static function validateExporterClass(string $class)
{
if (!class_exists($class)) {
throw new InvalidArgumentException('Could not find exporter class: ' . $class);
}
}
private static function getProtocolFromScheme(string $scheme): string
{
$components = explode('+', $scheme);
return count($components) === 1 ? $components[0] : $components[1];
}
private static function normalizeScheme(string $scheme): string
{
return str_replace('https', 'http', $scheme);
}
}

7
src/SDK/Trace/SpanExporter/LoggerExporter.php
View File

@ -65,15 +65,14 @@ class LoggerExporter implements SpanExporterInterface, LoggerAwareInterface
/**
* @param string $endpointUrl
* @param string $name
* @param string $args
* @param string|null $args
* @return LoggerExporter
*/
public static function fromConnectionString(string $endpointUrl, string $name, string $args): self
public static function fromConnectionString(string $endpointUrl, string $name, string $args = null): self
{
return new self(
$name,
new SimplePsrFileLogger($endpointUrl),
$args
new SimplePsrFileLogger($endpointUrl)
);
}

17
tests/Contrib/Unit/AbstractHttpExporterTest.php
View File

@ -26,6 +26,13 @@ abstract class AbstractHttpExporterTest extends AbstractExporterTest
*/
abstract public function createExporterWithDsn(string $dsn): SpanExporterInterface;
/**
* Must be implemented by concrete TestCases
*
* @return string
*/
abstract public function getExporterClass(): string;
public function createExporter(): SpanExporterInterface
{
return $this->createExporterWithDsn(static::EXPORTER_DSN);
@ -121,4 +128,14 @@ abstract class AbstractHttpExporterTest extends AbstractExporterTest
],
];
}
public function testFromConnectionString(): void
{
$exporterClass = static::getExporterClass();
$this->assertNotSame(
call_user_func([$exporterClass, 'fromConnectionString'], self::EXPORTER_DSN, $exporterClass, 'foo'),
call_user_func([$exporterClass, 'fromConnectionString'], self::EXPORTER_DSN, $exporterClass, 'foo')
);
}
}

5
tests/Contrib/Unit/JaegerExporterTest.php
View File

@ -25,4 +25,9 @@ class JaegerExporterTest extends AbstractHttpExporterTest
$this->getStreamFactoryInterfaceMock()
);
}
public function getExporterClass(): string
{
return Exporter::class;
}
}

5
tests/Contrib/Unit/NewrelicExporterTest.php
View File

@ -25,4 +25,9 @@ class NewrelicExporterTest extends AbstractHttpExporterTest
$this->getStreamFactoryInterfaceMock()
);
}
public function getExporterClass(): string
{
return Exporter::class;
}
}

8
tests/Contrib/Unit/OTLPGrpcExporterTest.php
View File

@ -222,4 +222,12 @@ class OTLPGrpcExporterTest extends AbstractExporterTest
return $mockClient;
}
public function testFromConnectionString(): void
{
$this->assertNotSame(
Exporter::fromConnectionString(),
Exporter::fromConnectionString()
);
}
}

8
tests/Contrib/Unit/OTLPHttpExporterTest.php
View File

@ -252,4 +252,12 @@ class OTLPHttpExporterTest extends AbstractExporterTest
'Grpc Scheme' => ['grpc://localhost:4317'],
];
}
public function testFromConnectionString(): void
{
$this->assertNotSame(
Exporter::fromConnectionString(),
Exporter::fromConnectionString()
);
}
}

5
tests/Contrib/Unit/ZipkinExporterTest.php
View File

@ -23,4 +23,9 @@ class ZipkinExporterTest extends AbstractHttpExporterTest
$this->getStreamFactoryInterfaceMock()
);
}
public function getExporterClass(): string
{
return Exporter::class;
}
}

5
tests/Contrib/Unit/ZipkinToNewrelicExporterTest.php
View File

@ -25,4 +25,9 @@ class ZipkinToNewrelicExporterTest extends AbstractHttpExporterTest
$this->getStreamFactoryInterfaceMock()
);
}
public function getExporterClass(): string
{
return Exporter::class;
}
}

77
tests/SDK/Unit/Trace/ExporterFactoryTest.php
View File

@ -8,11 +8,14 @@ use AssertWell\PHPUnitGlobalState\EnvironmentVariables;
use Exception;
use Http\Discovery\HttpClientDiscovery;
use Http\Discovery\Strategy\MockClientStrategy;
use OpenTelemetry\Contrib as Path;
use OpenTelemetry\Contrib;
use OpenTelemetry\SDK\Trace\ExporterFactory;
use OpenTelemetry\SDK\Trace\SpanExporter\ConsoleSpanExporter;
use PHPUnit\Framework\TestCase;
/**
* @covers \OpenTelemetry\SDK\Trace\ExporterFactory
*/
class ExporterFactoryTest extends TestCase
{
use EnvironmentVariables;
@ -28,70 +31,49 @@ class ExporterFactoryTest extends TestCase
}
/**
* @test
* @dataProvider endpointProvider
*/
public function exporterFactory_exporterHasCorrectEndpoint($name, $input, $expectedClass)
public function testExporterHasCorrectEndpoint($name, $input, $expectedClass): void
{
$factory = new ExporterFactory($name);
$exporter = $factory->fromConnectionString($input);
$this->assertInstanceOf($expectedClass, $exporter);
}
public function endpointProvider()
public function endpointProvider(): array
{
return [
'zipkin' => ['test.zipkin', 'zipkin+http://zipkin:9411/api/v2/spans', Path\Zipkin\Exporter::class],
'jaeger' => ['test.jaeger', 'jaeger+http://jaeger:9412/api/v2/spans', Path\Jaeger\Exporter::class],
'newrelic' => ['rest.newrelic', 'newrelic+https://trace-api.newrelic.com/trace/v1?licenseKey="23423423', Path\Newrelic\Exporter::class],
'otlp+http' => ['test.otlp', 'otlp+http://', Path\OtlpHttp\Exporter::class],
'otlp+grpc' => ['test.otlpgrpc', 'otlp+grpc://', Path\OtlpGrpc\Exporter::class],
'zipkintonewrelic' => ['test.zipkintonewrelic', 'zipkintonewrelic+https://trace-api.newrelic.com/trace/v1?licenseKey="23423423', Path\ZipkinToNewrelic\Exporter::class],
'console' => ['test.console', 'console+php://stdout', ConsoleSpanExporter::class],
'zipkin' => ['test.zipkin', 'zipkin+http://zipkin:9411/api/v2/spans', Contrib\Zipkin\Exporter::class],
'jaeger' => ['test.jaeger', 'jaeger+http://jaeger:9412/api/v2/spans', Contrib\Jaeger\Exporter::class],
'newrelic' => ['rest.newrelic', 'newrelic+https://trace-api.newrelic.com/trace/v1?licenseKey=abc23423423', Contrib\Newrelic\Exporter::class],
'otlp+http' => ['test.otlp', 'otlp+http', Contrib\OtlpHttp\Exporter::class],
'otlp+grpc' => ['test.otlpgrpc', 'otlp+grpc://otlp:4317', Contrib\OtlpGrpc\Exporter::class],
'zipkintonewrelic' => ['test.zipkintonewrelic', 'zipkintonewrelic+https://trace-api.newrelic.com/trace/v1?licenseKey=abc23423423', Contrib\ZipkinToNewrelic\Exporter::class],
'console' => ['test.console', 'console', ConsoleSpanExporter::class],
];
}
/**
* @test
* @dataProvider invalidConnectionStringProvider
*/
public function exporterFactory_invalidConnectionString(string $name, string $input)
public function testInvalidConnectionString(string $name, string $input): void
{
$this->expectException(Exception::class);
$factory = new ExporterFactory($name);
$factory->fromConnectionString($input);
}
public function invalidConnectionStringProvider()
public function invalidConnectionStringProvider(): array
{
return [
'zipkin without +' => ['test.zipkin', 'zipkinhttp://zipkin:9411/api/v2/spans'],
'zipkin with extra field' => ['test.zipkin', 'zipkin+http://zipkin:9411/api/v2/spans+extraField'],
'zapkin' => ['zipkin.test', 'zapkin+http://zipkin:9411/api/v2/spans'],
'otlp' => ['test.otlp', 'otlp'],
'test' => ['test', 'test+http://test:1345'],
];
}
/**
* @test
*/
public function testMissingLicenseKey()
{
$this->expectException(Exception::class);
$input = 'newrelic+https://trace-api.newrelic.com/trace/v1';
$factory = new ExporterFactory('test.newrelic');
$exporter = $factory->fromConnectionString($input);
$this->expectException(Exception::class);
$input = 'zipkintonewrelic+https://trace-api.newrelic.com/trace/v1';
$factory = new ExporterFactory('test.zipkintonewrelic');
$exporter = $factory->fromConnectionString($input);
}
/**
* @test
*/
public function exporterFactory_acceptsNoneExporter()
public function testAcceptsNoneExporterEnvVar()
{
$this->setEnvironmentVariable('OTEL_TRACES_EXPORTER', 'none');
$factory = new ExporterFactory('test.fromEnv');
@ -99,11 +81,10 @@ class ExporterFactoryTest extends TestCase
}
/**
* @test
* @dataProvider envProvider
* @psalm-param class-string $expected
*/
public function exporterFactory_createFromEnvironment(string $exporter, array $env, string $expected)
public function testCreateFromEnvironment(string $exporter, array $env, string $expected)
{
$this->setEnvironmentVariable('OTEL_TRACES_EXPORTER', $exporter);
foreach ($env as $k => $v) {
@ -113,28 +94,28 @@ class ExporterFactoryTest extends TestCase
$this->assertInstanceOf($expected, $factory->fromEnvironment());
}
public function envProvider()
public function envProvider(): array
{
return [
'otlp+http/protobuf from traces protocol' => [
'otlp',
['OTEL_EXPORTER_OTLP_TRACES_PROTOCOL' => 'http/protobuf'],
Path\OtlpHttp\Exporter::class,
Contrib\OtlpHttp\Exporter::class,
],
'otlp+http/protobuf from protocol' => [
'otlp',
['OTEL_EXPORTER_OTLP_PROTOCOL' => 'http/protobuf'],
Path\OtlpHttp\Exporter::class,
Contrib\OtlpHttp\Exporter::class,
],
'otlp+grpc from traces protocol' => [
'otlp',
['OTEL_EXPORTER_OTLP_TRACES_PROTOCOL' => 'grpc'],
Path\OtlpGrpc\Exporter::class,
Contrib\OtlpGrpc\Exporter::class,
],
'otlp+grpc from protocol' => [
'otlp',
['OTEL_EXPORTER_OTLP_PROTOCOL' => 'grpc'],
Path\OtlpGrpc\Exporter::class,
Contrib\OtlpGrpc\Exporter::class,
],
'console' => [
'console', [], ConsoleSpanExporter::class,
@ -143,10 +124,9 @@ class ExporterFactoryTest extends TestCase
}
/**
* @test
* @dataProvider invalidEnvProvider
*/
public function exporterFactory_throwsExceptionForInvalidOrUnsupportedExporterConfigs(string $exporter, array $env = [])
public function testThrowsExceptionForInvalidOrUnsupportedExporterConfigs(string $exporter, array $env = [])
{
$this->setEnvironmentVariable('OTEL_TRACES_EXPORTER', $exporter);
foreach ($env as $k => $v) {
@ -157,7 +137,7 @@ class ExporterFactoryTest extends TestCase
$factory->fromEnvironment();
}
public function invalidEnvProvider()
public function invalidEnvProvider(): array
{
return [
'jaeger' => ['jaeger'],
@ -177,4 +157,11 @@ class ExporterFactoryTest extends TestCase
'multiple exporters' => ['jaeger,zipkin'],
];
}
public function testNonExistingExporterEnvVar(): void
{
$this->expectException(Exception::class);
(new ExporterFactory())->fromEnvironment();
}
}