Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prototype: generate registry from composer autoload-dump #1412

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Oct 18, 2024

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:

  • components can be registered in various composer.json files, under extra.spi
  • existing _register.php files manually register through SPI
  • SDK registry fetches factories from SPI instead of maintaining static arrays
  • SDK Registry's various register... methods are deprecated and a no-op

If we move ahead with this approach, a follow-up PR could remove our various late-binding providers.

todo:

  • add config to appropriate composer.json files (currently only in root composer.json)

Copy link

netlify bot commented Oct 18, 2024

Deploy Preview for opentelemetry-php canceled.

Name Link
🔨 Latest commit 5ffe83b
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-php/deploys/671f1d38fe1753000839d348

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 25.74850% with 124 lines in your changes missing coverage. Please review.

Project coverage is 72.94%. Comparing base (fd654b9) to head (5ffe83b).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...tension/Propagator/B3/B3MultiPropagatorFactory.php 0.00% 6 Missing ⚠️
...rc/Extension/Propagator/B3/B3PropagatorFactory.php 0.00% 6 Missing ⚠️
...r/CloudTrace/CloudTraceOneWayPropagatorFactory.php 0.00% 6 Missing ⚠️
...pagator/CloudTrace/CloudTracePropagatorFactory.php 0.00% 6 Missing ⚠️
...opagator/Jaeger/JaegerBaggagePropagatorFactory.php 0.00% 6 Missing ⚠️
...sion/Propagator/Jaeger/JaegerPropagatorFactory.php 0.00% 6 Missing ⚠️
src/SDK/Registry.php 87.50% 5 Missing ⚠️
src/Contrib/Grpc/GrpcTransportFactory.php 0.00% 4 Missing ⚠️
src/Contrib/Otlp/LogsExporterFactory.php 0.00% 4 Missing ⚠️
src/Contrib/Otlp/MetricExporterFactory.php 0.00% 4 Missing ⚠️
... and 21 more
Additional details and impacted files

Impacted file tree graph

@@             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     
Flag Coverage Δ
8.1 72.55% <25.74%> (-0.87%) ⬇️
8.2 72.76% <25.74%> (-0.90%) ⬇️
8.3 72.80% <25.74%> (-0.90%) ⬇️
8.4 72.88% <25.74%> (-0.76%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/SDK/Common/Export/Http/PsrUtils.php 92.64% <100.00%> (ø)
src/SDK/Propagation/PropagatorFactory.php 95.00% <100.00%> (+0.88%) ⬆️
src/SDK/Trace/AutoRootSpan.php 85.71% <100.00%> (+0.60%) ⬆️
src/Contrib/Grpc/_register.php 0.00% <0.00%> (ø)
src/Contrib/Zipkin/_register.php 0.00% <0.00%> (ø)
src/SDK/Logs/Exporter/_register.php 0.00% <0.00%> (ø)
src/Contrib/Otlp/_register.php 0.00% <0.00%> (ø)
src/Extension/Propagator/B3/_register.php 0.00% <0.00%> (ø)
src/Extension/Propagator/CloudTrace/_register.php 0.00% <0.00%> (ø)
src/Extension/Propagator/Jaeger/_register.php 0.00% <0.00%> (ø)
... and 24 more

... and 199 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd654b9...5ffe83b. Read the comment docs.

@brettmc brettmc changed the title generate registry from composer autoload-dump ptototype: generate registry from composer autoload-dump Oct 20, 2024
@brettmc brettmc changed the title ptototype: generate registry from composer autoload-dump prototype: generate registry from composer autoload-dump Oct 20, 2024
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 brettmc force-pushed the composer-extra-registry branch from ffb28df to abd9944 Compare October 21, 2024 00:48
@Nevay
Copy link
Contributor

Nevay commented Oct 21, 2024

An alternative idea if we want to avoid duplicating the "load implementations from composer.json" approach:

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 composer.json configuration. This information could instead be added as additional method in the interfaces1 to allow loading them using SPI.

Registry is currently used for two different types of implementations:

  • factories used to initialize the SDK from environment variables
  • transport factory implementations, which are also used outside of environment variables configuration

Transport factory implementations

We can extend TransportFactoryInterface with the necessary information to populate the transport factories:

$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 variables

We could align the implementation with the file-based implementation by adding a new, from the current implementation independent interface2 similar to Configuration\ComponentProvider and deprecate the current registry methods and factory interfaces.
Very basic interface definition:

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 Registry.

Footnotes

  1. See also Java ServiceLoader - Designing Services:

    A service should declare as many methods as needed to allow service providers to communicate their domain-specific properties and other quality-of-implementation factors. An application which obtains a service loader for the service may then invoke these methods on each instance of a service provider, in order to choose the best provider for the application.

  2. FWIW I've been using the following Env\Loader interface for env-based configuration, its implementations are loaded via SPI:

    /**
     * @template T
     */
    interface Loader {
    
        /**
         * @return T
         */
        public function load(EnvResolver $env, LoaderRegistry $registry, Context $context): mixed;
    
        public function name(): string;
    }
    

@brettmc
Copy link
Collaborator Author

brettmc commented Oct 22, 2024

updated to use SPI, and implemented the idea of having factories declare their key & priority.

@haad
Copy link

haad commented Oct 23, 2024

  • The Vneno i

Copy link
Contributor

@ChrisLightfootWild ChrisLightfootWild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an exciting set of changes!

Just left a few bits for consideration/discussion.

Comment on lines 46 to 48
if ($compression === 'none') {
$compression = null;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be better suited within PsrUtils::compression()?

Comment on lines +12 to +13
public function type(): string;
public function priority(): int;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above with an example from the Zipkin exporter.

Comment on lines +33 to +34
public function type(): string;
public function priority(): int;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be considering this to be a breaking change? 🤔

Example in exporter-otlp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be considering this to be a breaking change? 🤔

Comment on lines +10 to +11
public function type(): string;
public function priority(): int;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

Comment on lines 27 to 28
}
public function priority(): int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
public function priority(): int
}
public function priority(): int

Sorry 😬
Hopefully there's a lint setting somewhere so I don't look so pedantic 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor

@xvilo xvilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/Extension/Propagator/B3/B3MultiPropagatorFactory.php Outdated Show resolved Hide resolved
@brettmc
Copy link
Collaborator Author

brettmc commented Oct 23, 2024

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?

@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.

Comment on lines +126 to +136
"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"
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you prefer the explicit list or can we just defer to auto-pruning?

Suggested change
"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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants