auto-instrumentation registration #1304
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1304 +/- ##
============================================
+ Coverage 74.28% 74.38% +0.10%
- Complexity 2491 2539 +48
============================================
Files 353 365 +12
Lines 7135 7274 +139
============================================
+ Hits 5300 5411 +111
- Misses 1835 1863 +28
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
src/API/Instrumentation/AutoInstrumentation/Instrumentation.php
Outdated
Show resolved
Hide resolved
src/API/Instrumentation/AutoInstrumentation/ExtensionHookManager.php
Outdated
Show resolved
Hide resolved
src/API/Instrumentation/AutoInstrumentation/ExtensionHookManager.php
Outdated
Show resolved
Hide resolved
- deptrac was rightly complaining that API (indirectly) depended on SDK through config/sdk. For now, remove usage of Config\SDK\Configuration\Context - update deptrac config to allow some new dependencies
psalm doesn't complain now, so that should be good
- make "register global" a function of Sdk, but keep the sdk builder's interface intact - invent an API instrumentation context, similar to the one in config/sdk, to pass providers to instrumentations - add an initial test of autoloading from a config file
in passing, remove some dead code for handling invalid booleans - config already handles this correctly
src/API/Instrumentation/AutoInstrumentation/ExtensionHookManager.php
Outdated
Show resolved
Hide resolved
allow SDK created from config file to coexist with components using globals initializer
src/SDK/SdkAutoloader.php
Outdated
| { | ||
| /** @var HookManager $hookManager */ | ||
| foreach (ServiceLoader::load(HookManager::class) as $hookManager) { | ||
| $scope = $hookManager->enable(Context::getCurrent())->activate(); |
There was a problem hiding this comment.
I dislike this as it does not guarantee correct detach order for the scope. Also might lead to issues if Context::setStorage() is called afterwards. We could add a method that allows changing hook managers from default disabled to default enabled which would make this scope obsolete.
There was a problem hiding this comment.
We could add a method that allows changing hook managers from default disabled to default enabled which would make this scope obsolete.
I do think it would be preferable if the hook manager was enabled by default. If you've installed auto-instrumentation packages, you should get auto-instrumentation (which is the current behaviour). Plus, I don't want developers to need to write code to enable hooks to run; the most common usage I think would be to instrument.
There was a problem hiding this comment.
I've switched ExtensionHookManager to being enabled by default. How one might access the hook manager to disable it isn't very clear if it was autoloaded - maybe it needs to be a singleton or registered in context?
src/SDK/SdkAutoloader.php
Outdated
| if ($ignoreUrls === []) { | ||
| return false; | ||
| if (!Configuration::has(Variables::OTEL_PHP_INSTRUMENTATION_CONFIG_FILE)) { | ||
| return; |
There was a problem hiding this comment.
IMO we should load instrumentations with empty configuration registry if the config file is not specified.
Edit: not possible yet as autoload.files might register factories using OpenTelemetry\SDK\Registry::registerXxxFactory()?
There was a problem hiding this comment.
It seems to work. autoload.files should not be an issue since everything continues to use globals initializers.
I've wrapped registering instrumentation in a try/catch/log, since some will not work without config. If we could work out a way to provide the default config rather than empty (ie, by processing the InstrumentationConfiguration without input), it might work a bit better?
There was a problem hiding this comment.
If we could work out a way to provide the default config rather than empty (ie, by processing the
InstrumentationConfigurationwithout input), it might work a bit better?
This is the bit I'm watching out for too, as I'm trying to keep existing behaviour locally without providing config files.
There was a problem hiding this comment.
One solution could be to extend the InstrumentationConfiguration interface so that each config class can provide its own defaults:
interface InstrumentationConfiguration
{
public static function default(): self;
}
class ExampleInstrumentation implements InstrumentationConfiguration
{
public static function default(): self
{
return new self('default1', 'another-default');
}
}
public function register(HookManager $hookManager, ConfigurationRegistry $configuration, InstrumentationContext $context): void
{
$config = $configuration->get(ExampleConfig::class) ?? ExampleConfig::default();
//snip
}
}- drop storage from hook manager: can't guarantee that something else, eg swoole, won't modify storage - drop context from hook manager: we must lazy-load globals via initializers, because not all instrumentations use SPI (although that may change in future) - default hook manager to enabled
if no config provided, still try to load them. wrap registration in a try/catch/log
| return; | ||
| } | ||
|
|
||
| $tracer = Globals::tracerProvider()->getTracer('example-instrumentation'); |
There was a problem hiding this comment.
This triggers initialization during sdk autoloading; iff we want to use the Globals providers to delay initialization, we should specify that Globals must not be accessed outside of hook callbacks. Otherwise I don't see an advantage over injecting the providers.
Apart from that I would generally prefer if we inject the providers. It allows initializing telemetry instances outside of hook callbacks¹ and might allow us to change Globals to be a truly global state instead of being Context dependent.
¹ For example reusing a counter instance and calling ::add() directly is around three times faster than refetching the internal instrument from a CachedInstrumentation instance. (initial counter creation w/ single metric reader/metric stream takes ~33μs):
+---------------------------------+---------+----------+--------+---------+
| subject | memory | mode | rstdev | stdev |
+---------------------------------+---------+----------+--------+---------+
| benchCounterAdd (empty) | 2.716mb | 0.902μs | ±1.95% | 0.018μs |
| benchCounterAddRecreate (empty) | 2.780mb | 3.319μs | ±1.86% | 0.062μs |
| benchCounterAdd (http) | 2.727mb | 1.954μs | ±1.63% | 0.032μs |
| benchCounterAddRecreate (http) | 2.783mb | 4.539μs | ±1.73% | 0.079μs |
+---------------------------------+---------+----------+--------+---------+
There was a problem hiding this comment.
That makes sense, and I prefer the method of passing in the providers when registering an instrumentation. I don't think we can commit to file-based config and deprecate Registry just yet, given the experimental state of config.
But, I think we can start to move in that direction. How does this sound?
- update existing instrumentation classes to only retrieve providers from hook callbacks
- document this new restriction (probably in
Globals) - add a todo to Registry to deprecate in a future release (in case anybody's reading the source)
- pass in Noop providers to
Instrumentations via::register(), and document that these will become real implementations in a future release but to keep usingGlobalsfor now
We then wait for config to stabilise.
There was a problem hiding this comment.
I don't think we can commit to file-based config and deprecate Registry just yet, given the experimental state of config.
The restriction / issue with Registry is not related to file-based config; any usage of Globals within an autoload.files file might trigger this issue already, for example open-telemetry/opentelemetry-auto-openai-php.
- If we have to stay compatible with
Registry::register{Xxx}Factoryfor now:
Instead of noop providers we could inject providers that resolveGlobalson the first call to::spanBuilder()/::create{Instrument}()/::emit(). This would allow us to use the injected providers immediately (with the same restriction asCachedInstrumentationthatTracer/Meter/Loggermethods must only be called within hook callbacks for now).
TracerProvider example
$tracerProvider = new LateBindingTracerProvider(static function(): TracerProviderInterface {
$scope = Context::getRoot()->activate();
try {
return Globals::tracerProvider();
} finally {
$scope->detach();
}
});
final class LateBindingTracerProvider implements TracerProviderInterface {
private ?TracerProviderInterface $tracerProvider = null;
/** @param Closure(): TracerProviderInterface $factory */
public function __construct(
private readonly Closure $factory,
) {}
public function getTracer(string $name, ?string $version = null, ?string $schemaUrl = null, iterable $attributes = []): TracerInterface {
return $this->tracerProvider?->getTracer($name, $version, $schemaUrl, $attributes)
?? new LateBindingTracer(fn(): TracerInterface => ($this->tracerProvider ??= ($this->factory)())->getTracer($name, $version, $schemaUrl, $attributes));
}
}
final class LateBindingTracer implements TracerInterface {
private ?TracerInterface $tracer = null;
/** @param Closure(): TracerInterface $factory */
public function __construct(
private readonly Closure $factory,
) {}
public function spanBuilder(string $spanName): SpanBuilderInterface {
return ($this->tracer ??= ($this->factory)())->spanBuilder($spanName);
}
}There was a problem hiding this comment.
Implemented late-binding providers based on the above example (thanks!)
per review from Nevay, this will allow instrumentations to get things from Globals as late as possible
not available in phpunit 10, so comment out and leave a todo
| // EXAMPLE_INSTRUMENTATION_SPAN_NAME=test1234 php examples/instrumentation/configure_instrumentation_global.php | ||
| putenv('OTEL_PHP_AUTOLOAD_ENABLED=true'); | ||
| putenv('OTEL_EXPERIMENTAL_CONFIG_FILE=examples/instrumentation/otel-sdk.yaml'); | ||
| putenv('OTEL_PHP_INSTRUMENTATION_CONFIG_FILE=examples/instrumentation/otel-instrumentation.yaml'); |
There was a problem hiding this comment.
NB that I'm working on a PR over in the config repo to incorporate instrumentation into the general file-based config. I did a quick test against this branch, and combining them was a pretty minor change.
OTEL_PHP_INSTRUMENTATION_CONFIG_FILEto tell the autoloader where the config files are for auto-instrumentationOTEL_EXPERIMENTAL_CONFIG_FILEdefines where the SDK config file resides (relative to CWD), and also acts as a trigger to autoload via SPI rather than our current env-based versiontodo: