prototype: generate registry from composer autoload-dump #1412
Conversation
✅ Deploy Preview for opentelemetry-php canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1412 +/- ##
============================================
- Coverage 73.73% 72.94% -0.79%
- Complexity 2683 2718 +35
============================================
Files 387 392 +5
Lines 7672 8022 +350
============================================
+ Hits 5657 5852 +195
- Misses 2015 2170 +155
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 199 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
brettmc
changed the title
brettmc
changed the title
To resolve our long-standing race conditions stemming from using composer's autoload->files to registry SDK components at runtime, this changes things so that: - components are registed in various composer.json files, under the extra.opentelemetry.registry key - a composer script is registered to write this config out to a JSON file - the SDK Registry is modified to make manually registering components a no-op (currently behind a flag, OTEL_PHP_EXPERIMENTAL_JSON_REGISTRY), and instead configure itself from the generated JSON file If we move ahead with this approach, a follow-up PR could tidy up the Registry and remove our various late-binding providers.
brettmc
force-pushed
the
composer-extra-registry
branch
from
ffb28df
to
abd9944
Compare
|
An alternative idea if we want to avoid duplicating the "load implementations from The main difference between the solution in this PR and an SPI-based approach is the addition of a name/key for each implementation within the
Transport factory implementationsWe can extend $factories = iterator_to_array(ServiceLoader::load(TransportFactoryServiceInterface::class));
array_multisort(
array_map(static fn($factory) => $factory->priority(), $factories),
SORT_DESC,
$factories,
);
$factoriesByProtocol = [];
foreach ($factories as $factory) {
$factoriesByProtocol[$factory->protocol()] ??= $factory;
}
self::$transportFactories = $factoriesByProtocol;interface TransportFactoryServiceInterface extends TransportFactoryInterface {
public function protocol(): string;
public function priority(): int;
}Factories to initialize the SDK from environment variablesWe could align the implementation with the file-based implementation by adding a new, from the current implementation independent interface2 similar to namespace OpenTelemetry\Config\SDK\Environment;
/**
* @template T
*/
interface ComponentProvider {
/**
* @return T
*/
public function createPlugin(): mixed;
public function name(): string;
}Alternatively we could follow the approach mentioned for transport factory implementations and add a name(/priority) method to the factory interfaces and continue using the Footnotes
|
|
updated to use SPI, and implemented the idea of having factories declare their key & priority. |
|
| if ($compression === 'none') { | ||
| $compression = null; | ||
| } |
There was a problem hiding this comment.
Would this be better suited within PsrUtils::compression()?
| public function type(): string; | ||
| public function priority(): int; |
There was a problem hiding this comment.
As above with an example from the Zipkin exporter.
| public function type(): string; | ||
| public function priority(): int; |
There was a problem hiding this comment.
Should we be considering this to be a breaking change? 🤔
Example in exporter-otlp.
There was a problem hiding this comment.
This is a good point, we should consider trying to do this in a non-breaking way. Perhaps another level of abstraction like a TransportFactoryProvider & friends which has these extra methods and returns a factory 🤔
| @@ -7,4 +7,6 @@ | |||
| interface LogRecordExporterFactoryInterface | |||
| { | |||
| public function create(): LogRecordExporterInterface; | |||
| public function type(): string; | |||
There was a problem hiding this comment.
Should we be considering this to be a breaking change? 🤔
| public function type(): string; | ||
| public function priority(): int; |
| } | ||
| public function priority(): int |
There was a problem hiding this comment.
| } | |
| public function priority(): int | |
| } | |
| public function priority(): int |
Sorry 😬
Hopefully there's a lint setting somewhere so I don't look so pedantic 😅
There was a problem hiding this comment.
there is a fixer that can do this:
'class_attributes_separation' => ['elements' => ['method' => 'one'],]
It changes a lot of files, so I won't do it in this PR, but did fix the blanks b/w priority()
There was a problem hiding this comment.
One small code-style comment was placed.
I understand that the Registry stuff will not be used when using extra.spi. Is it correct that third-party libraries (in my case, proprietary code/packages within a company) can also add their instrumentation through the SPI way, as shown in composer.json?
It would be nice to pair this with some documentation regardless, e.g. a 'How to add instrumentation into your composer package' section. It would be a good way to demo/reflect/document the preferred way of including AutoInstrumentations.
@xvilo That's right. It's still up for debate exactly how registry/SPI should interact, and which one would be the default. The SPI mechanism will definitely allow custom components like we have now. |
| "prune-autoload-files": [ | ||
| "src/Contrib/Otlp/_register.php", | ||
| "src/Contrib/Grpc/_register.php", | ||
| "src/Contrib/Zipkin/_register.php", | ||
| "src/Extension/Propagator/B3/_register.php", | ||
| "src/Extension/Propagator/CloudTrace/_register.php", | ||
| "src/Extension/Propagator/Jaeger/_register.php", | ||
| "src/SDK/Logs/Exporter/_register.php", | ||
| "src/SDK/Metrics/MetricExporter/_register.php", | ||
| "src/SDK/Trace/SpanExporter/_register.php" | ||
| ] |
There was a problem hiding this comment.
Do you prefer the explicit list or can we just defer to auto-pruning?
| "prune-autoload-files": [ | |
| "src/Contrib/Otlp/_register.php", | |
| "src/Contrib/Grpc/_register.php", | |
| "src/Contrib/Zipkin/_register.php", | |
| "src/Extension/Propagator/B3/_register.php", | |
| "src/Extension/Propagator/CloudTrace/_register.php", | |
| "src/Extension/Propagator/Jaeger/_register.php", | |
| "src/SDK/Logs/Exporter/_register.php", | |
| "src/SDK/Metrics/MetricExporter/_register.php", | |
| "src/SDK/Trace/SpanExporter/_register.php" | |
| ] | |
| "prune-autoload-files": true |
To resolve our long-standing race conditions stemming from using composer's autoload->files to register SDK components at runtime, this changes things so that:
extra.spi_register.phpfiles manually register through SPIregister...methods are deprecated and a no-opIf we move ahead with this approach, a follow-up PR could remove our various late-binding providers.
todo: