implement scope configuration #1353
Conversation
spec 1.34 requires logger, tracer, and instrument to have an enabled function
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1353 +/- ##
============================================
- Coverage 74.19% 74.01% -0.19%
- Complexity 2584 2641 +57
============================================
Files 373 379 +6
Lines 7430 7547 +117
============================================
+ Hits 5513 5586 +73
- Misses 1917 1961 +44
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -143,6 +148,12 @@ public function collectAndPush(iterable $streamIds): void | |||
| $aggregator = $this->asynchronousAggregatorFactories[$streamId]->create(); | |||
|
|
|||
| $instrumentId = $this->streamToInstrument[$streamId]; | |||
| if ( | |||
| array_key_exists($instrumentId, $this->instruments) | |||
| && $this->instruments[$instrumentId]->meter?->isEnabled() === false | |||
There was a problem hiding this comment.
Should be moved into MetricProducer (/MetricReader) if we don't want to export synchronous metrics of disabled meters.
We could also remove the underlying metric streams but this would require more effort.
There was a problem hiding this comment.
Adjusted MetricReader to not export when source->instrument->meter is not enabled. I've left the above code in place for now, since it also acts to stop observer callbacks from being called (which seems like a performance win, if those callbacks were computationally expensive) - happy to revisit that, though!
There was a problem hiding this comment.
since it also acts to stop observer callbacks from being called
We should also skip collecting synchronous aggregators which could be done by moving the $this->instruments[$instrumentId]->meter->isEnabled() check to the beginning of the for-loop and populating $this->instruments in ::registerSynchronousStream()/::registerAsynchronousStream() instead of ::registerCallback() (could alternatively be done in ExportingReader::doCollect() by removing disabled meters from the stream ids that are passed to ::collectAndPush(), but keeping it in MetricRegistry seems easier with the current setup). But...
We could also remove the underlying metric streams but this would require more effort.
This seems like the only way to implement it properly, otherwise we will export incorrect data when instrumentation scopes are reenabled.
# w/ temporality=cumulative
$c = $meterProvider->getMeter('test')->createCounter('c');
# t0
$meterConfig->setDisabled(false);
$c->add(1);
# t1
$meterConfig->setDisabled(true);
$c->add(1);
# t2, {sum=1, startTimestamp=t2}; must not export {sum=2, startTimestamp=t0}
$meterConfig->setDisabled(false);
$c->add(1);
$meterProvider->forceFlush();Very rough implementation idea (alternatively see implementation with slightly different metrics SDK):
- on disable: remove streams for all instrumentation scope instruments (within
Meter:$this->instruments->writersand->observers); logic similar to stalenesshandler callbacks inMeterandExportingReaderwhich already remove streams - on enable: recreate streams for all instrumentation scope instruments by calling
MetricFactory::createSynchronousWriter()/::createAsynchronousObserver() Meter::createSynchronousWriter()/::createAsynchronousObserver()must not create streams if the instrumentation scope is disabled
There was a problem hiding this comment.
I haven't been able to get this to work, but have left a todo and a skipped integration.
| { | ||
| foreach ($this->conditions as $condition) { | ||
| if ($condition->matches($scope)) { | ||
| return new Config($condition->state()); |
There was a problem hiding this comment.
How will we add new configuration options if the spec is extended (esp. if signal specific options are added; do we want a dedicated config class per signal?)?
I'm currently playing around with an alternative approach that uses callbacks to modify the config rather than using the config state that is returned by the first matching condition; all matching callbacks are applied to the initial config, which would make supporting new config options trivial. The configurator uses a WeakMap<InstrumentationScope, TConfig> to update configs when a new callback is registered.
(new Configurator(static fn() => new MeterConfig()))
->with(static fn(MeterConfig $config) => $config->disabled = true, name: 'io.opentelemetry.contrib.*')
->with(static fn(MeterConfig $config) => $config->disabled = true, name: 'my-meter')There was a problem hiding this comment.
I think that we do, or will, need a config class per signal. I was just trying to avoid copy-pasting the same code when they currently have the same functionality.
I don't mind addressing that sooner than later - either I do just turn the one general class into 3 signal-specific ones, or I'm happy to incorporate your alternative if you've got some more code to point me at?
There was a problem hiding this comment.
I was using the following Configurator implementation, with TracerProvider/MeterProvider/LoggerProvider accepting a Closure(InstrumentationScope): TConfig as configurator ("[Tracer/Meter/Logger]Configurator is modeled as a function to maximize flexibility.").
Configurator implementation
I'm not too happy with the state of this implementation as it does not support removal of callbacks yet.
Might want to add a predicate parameter to ::with() that allows flexible conditions.
/**
* @template T
*/
final class Configurator {
/** @var Closure(InstrumentationScope): T */
private readonly Closure $factory;
/** @var WeakMap<InstrumentationScope, T> */
private WeakMap $configs;
/** @var list<ConfiguratorClosure> */
private array $configurators = [];
/**
* @param Closure(InstrumentationScope): T $factory
*/
public function __construct(Closure $factory) {
$this->configs = new WeakMap();
$this->factory = $factory;
}
/**
* @param Closure(T, InstrumentationScope): void $closure
*/
public function with(Closure $closure, ?string $name, ?string $version = null, ?string $schemaUrl = null): self {
$this->configurators[] = $configurator = new ConfiguratorClosure($closure, self::namePattern($name), $version, $schemaUrl);
foreach ($this->configs as $instrumentationScope => $config) {
if ($configurator->matches($instrumentationScope)) {
($configurator->closure)($config, $instrumentationScope);
}
}
return $this;
}
/**
* @return T
*/
public function resolve(InstrumentationScope $instrumentationScope): object {
if ($config = $this->configs[$instrumentationScope] ?? null) {
return $config;
}
$config = ($this->factory)($instrumentationScope);
foreach ($this->configurators as $configurator) {
if ($configurator->matches($instrumentationScope)) {
($configurator->closure)($config, $instrumentationScope);
}
}
return $this->configs[$instrumentationScope] ??= $config;
}
private static function namePattern(?string $name): ?string {
return $name !== null
? sprintf('/^%s$/', strtr(preg_quote($name, '/'), ['\\?' => '.', '\\*' => '.*']))
: null;
}
}
/**
* @internal
*/
final class ConfiguratorClosure {
public function __construct(
public readonly Closure $closure,
private readonly ?string $name,
private readonly ?string $version,
private readonly ?string $schemaUrl,
) {}
public function matches(InstrumentationScope $instrumentationScope): bool {
return ($this->name === null || preg_match($this->name, $instrumentationScope->name))
&& ($this->version === null || $this->version === $instrumentationScope->version)
&& ($this->schemaUrl === null || $this->schemaUrl === $instrumentationScope->schemaUrl);
}
}$meterProviderBuilder->setMeterConfigurator((new Configurator(static fn() => new MeterConfig()))
->with(static fn(MeterConfig $config) => $config->setDisabled(true), name: '*')
->with(static fn(MeterConfig $config) => $config->setDisabled(false), name: 'test')
->resolve(...)):There was a problem hiding this comment.
Updated the Configurator implementation to use the above. I had to suppress a couple of phan/phpstan complaints about templates, but it works. I added a couple of convenience methods to create a default Configurator for each signal (Configurator::meter() etc).
I think it's also worth noting/documenting that if there are multiple matching rules in a configurator, then the last one wins.
the feature is not implemented yet
use Nevay's implementation, provided in PR feedback
|
@open-telemetry/php-approvers I think this is ready for review now. |
Implement the "scope config" changes specified in open-telemetry/opentelemetry-specification#3877
enabledtoisEnabled, since the spec didn't actually specify what the methods had to be called, and isEnabled seems nicerCloses: #1349