From d3765662f94919094fb36a0022941ee1598d2b53 Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Wed, 13 Dec 2023 03:03:10 +0100 Subject: [PATCH] Add instrument advisory parameter (#1186) * Add instrument advisory parameter * Undo bucket boundaries change to preserve BC "SDKs SHOULD use the default value when boundaries are not explicitly provided, unless they have good reasons to use something different (e.g. for backward compatibility reasons in a stable SDK release)." * Add note that passing callback instead of advisory is deprecated --- src/API/Metrics/MeterInterface.php | 22 ++++++-- src/API/Metrics/Noop/NoopMeter.php | 12 ++--- src/API/composer.json | 3 ++ .../DefaultAggregationProviderInterface.php | 6 ++- .../DefaultAggregationProviderTrait.php | 4 +- src/SDK/Metrics/Instrument.php | 8 ++- src/SDK/Metrics/Meter.php | 50 ++++++++++++++----- .../Metrics/MetricReader/ExportingReader.php | 7 +-- tests/Unit/SDK/Metrics/MeterTest.php | 31 ++++++++++++ .../MetricReader/ExportingReaderTest.php | 11 ++++ 10 files changed, 126 insertions(+), 28 deletions(-) diff --git a/src/API/Metrics/MeterInterface.php b/src/API/Metrics/MeterInterface.php index 6e06d9085..d7b0c9001 100644 --- a/src/API/Metrics/MeterInterface.php +++ b/src/API/Metrics/MeterInterface.php @@ -13,6 +13,7 @@ interface MeterInterface * @param string $name name of the instrument * @param string|null $unit unit of measure * @param string|null $description description of the instrument + * @param array $advisory an optional set of recommendations * @return CounterInterface created instrument * * @see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#counter-creation @@ -20,7 +21,8 @@ interface MeterInterface public function createCounter( string $name, ?string $unit = null, - ?string $description = null + ?string $description = null, + array $advisory = [] ): CounterInterface; /** @@ -29,6 +31,8 @@ public function createCounter( * @param string $name name of the instrument * @param string|null $unit unit of measure * @param string|null $description description of the instrument + * @param array|callable $advisory an optional set of recommendations, or + * deprecated: the first callback to report measurements * @param callable ...$callbacks responsible for reporting measurements * @return ObservableCounterInterface created instrument * @@ -38,6 +42,7 @@ public function createObservableCounter( string $name, ?string $unit = null, ?string $description = null, + $advisory = [], callable ...$callbacks ): ObservableCounterInterface; @@ -47,6 +52,8 @@ public function createObservableCounter( * @param string $name name of the instrument * @param string|null $unit unit of measure * @param string|null $description description of the instrument + * @param array $advisory an optional set of recommendations, e.g. + * ['ExplicitBucketBoundaries' => [0.25, 0.5, 1, 5]] * @return HistogramInterface created instrument * * @see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#histogram-creation @@ -54,7 +61,8 @@ public function createObservableCounter( public function createHistogram( string $name, ?string $unit = null, - ?string $description = null + ?string $description = null, + array $advisory = [] ): HistogramInterface; /** @@ -63,6 +71,8 @@ public function createHistogram( * @param string $name name of the instrument * @param string|null $unit unit of measure * @param string|null $description description of the instrument + * @param array|callable $advisory an optional set of recommendations, or + * deprecated: the first callback to report measurements * @param callable ...$callbacks responsible for reporting measurements * @return ObservableGaugeInterface created instrument * @@ -72,6 +82,7 @@ public function createObservableGauge( string $name, ?string $unit = null, ?string $description = null, + $advisory = [], callable ...$callbacks ): ObservableGaugeInterface; @@ -81,6 +92,7 @@ public function createObservableGauge( * @param string $name name of the instrument * @param string|null $unit unit of measure * @param string|null $description description of the instrument + * @param array $advisory an optional set of recommendations * @return UpDownCounterInterface created instrument * * @see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#updowncounter-creation @@ -88,7 +100,8 @@ public function createObservableGauge( public function createUpDownCounter( string $name, ?string $unit = null, - ?string $description = null + ?string $description = null, + array $advisory = [] ): UpDownCounterInterface; /** @@ -97,6 +110,8 @@ public function createUpDownCounter( * @param string $name name of the instrument * @param string|null $unit unit of measure * @param string|null $description description of the instrument + * @param array|callable $advisory an optional set of recommendations, or + * deprecated: the first callback to report measurements * @param callable ...$callbacks responsible for reporting measurements * @return ObservableUpDownCounterInterface created instrument * @@ -106,6 +121,7 @@ public function createObservableUpDownCounter( string $name, ?string $unit = null, ?string $description = null, + $advisory = [], callable ...$callbacks ): ObservableUpDownCounterInterface; } diff --git a/src/API/Metrics/Noop/NoopMeter.php b/src/API/Metrics/Noop/NoopMeter.php index 31e3bd35c..27f36e73f 100644 --- a/src/API/Metrics/Noop/NoopMeter.php +++ b/src/API/Metrics/Noop/NoopMeter.php @@ -14,32 +14,32 @@ final class NoopMeter implements MeterInterface { - public function createCounter(string $name, ?string $unit = null, ?string $description = null): CounterInterface + public function createCounter(string $name, ?string $unit = null, ?string $description = null, array $advisory = []): CounterInterface { return new NoopCounter(); } - public function createObservableCounter(string $name, ?string $unit = null, ?string $description = null, callable ...$callbacks): ObservableCounterInterface + public function createObservableCounter(string $name, ?string $unit = null, ?string $description = null, $advisory = [], callable ...$callbacks): ObservableCounterInterface { return new NoopObservableCounter(); } - public function createHistogram(string $name, ?string $unit = null, ?string $description = null): HistogramInterface + public function createHistogram(string $name, ?string $unit = null, ?string $description = null, array $advisory = []): HistogramInterface { return new NoopHistogram(); } - public function createObservableGauge(string $name, ?string $unit = null, ?string $description = null, callable ...$callbacks): ObservableGaugeInterface + public function createObservableGauge(string $name, ?string $unit = null, ?string $description = null, $advisory = [], callable ...$callbacks): ObservableGaugeInterface { return new NoopObservableGauge(); } - public function createUpDownCounter(string $name, ?string $unit = null, ?string $description = null): UpDownCounterInterface + public function createUpDownCounter(string $name, ?string $unit = null, ?string $description = null, array $advisory = []): UpDownCounterInterface { return new NoopUpDownCounter(); } - public function createObservableUpDownCounter(string $name, ?string $unit = null, ?string $description = null, callable ...$callbacks): ObservableUpDownCounterInterface + public function createObservableUpDownCounter(string $name, ?string $unit = null, ?string $description = null, $advisory = [], callable ...$callbacks): ObservableUpDownCounterInterface { return new NoopObservableUpDownCounter(); } diff --git a/src/API/composer.json b/src/API/composer.json index 39acaec47..505308ff0 100644 --- a/src/API/composer.json +++ b/src/API/composer.json @@ -24,6 +24,9 @@ "symfony/polyfill-php81": "^1.26", "symfony/polyfill-php82": "^1.26" }, + "conflict": { + "open-telemetry/sdk": "<=1.0.1" + }, "autoload": { "psr-4": { "OpenTelemetry\\API\\": "." diff --git a/src/SDK/Metrics/DefaultAggregationProviderInterface.php b/src/SDK/Metrics/DefaultAggregationProviderInterface.php index e5c2fcc0c..f2405e92d 100644 --- a/src/SDK/Metrics/DefaultAggregationProviderInterface.php +++ b/src/SDK/Metrics/DefaultAggregationProviderInterface.php @@ -8,6 +8,10 @@ interface DefaultAggregationProviderInterface { /** * @param string|InstrumentType $instrumentType + * @param array $advisory optional set of recommendations + * + * @noinspection PhpDocSignatureInspection not added for BC + * @phan-suppress PhanCommentParamWithoutRealParam @phpstan-ignore-next-line */ - public function defaultAggregation($instrumentType): ?AggregationInterface; + public function defaultAggregation($instrumentType /*, array $advisory = [] */): ?AggregationInterface; } diff --git a/src/SDK/Metrics/DefaultAggregationProviderTrait.php b/src/SDK/Metrics/DefaultAggregationProviderTrait.php index 37c5cb110..737ec40dd 100644 --- a/src/SDK/Metrics/DefaultAggregationProviderTrait.php +++ b/src/SDK/Metrics/DefaultAggregationProviderTrait.php @@ -6,7 +6,7 @@ trait DefaultAggregationProviderTrait { - public function defaultAggregation($instrumentType): ?AggregationInterface + public function defaultAggregation($instrumentType, array $advisory = []): ?AggregationInterface { switch ($instrumentType) { case InstrumentType::COUNTER: @@ -16,7 +16,7 @@ public function defaultAggregation($instrumentType): ?AggregationInterface case InstrumentType::ASYNCHRONOUS_UP_DOWN_COUNTER: return new Aggregation\SumAggregation(); case InstrumentType::HISTOGRAM: - return new Aggregation\ExplicitBucketHistogramAggregation([0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]); + return new Aggregation\ExplicitBucketHistogramAggregation($advisory['ExplicitBucketBoundaries'] ?? [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]); case InstrumentType::ASYNCHRONOUS_GAUGE: return new Aggregation\LastValueAggregation(); } diff --git a/src/SDK/Metrics/Instrument.php b/src/SDK/Metrics/Instrument.php index 3543604c0..b696a41e1 100644 --- a/src/SDK/Metrics/Instrument.php +++ b/src/SDK/Metrics/Instrument.php @@ -23,14 +23,20 @@ final class Instrument * @readonly */ public ?string $description; + /** + * @readonly + */ + public array $advisory; + /** * @param string|InstrumentType $type */ - public function __construct($type, string $name, ?string $unit, ?string $description) + public function __construct($type, string $name, ?string $unit, ?string $description, array $advisory = []) { $this->type = $type; $this->name = $name; $this->unit = $unit; $this->description = $description; + $this->advisory = $advisory; } } diff --git a/src/SDK/Metrics/Meter.php b/src/SDK/Metrics/Meter.php index 902284839..7bdcc5aa2 100644 --- a/src/SDK/Metrics/Meter.php +++ b/src/SDK/Metrics/Meter.php @@ -4,7 +4,9 @@ namespace OpenTelemetry\SDK\Metrics; +use function array_unshift; use ArrayAccess; +use function is_callable; use OpenTelemetry\API\Metrics\CounterInterface; use OpenTelemetry\API\Metrics\HistogramInterface; use OpenTelemetry\API\Metrics\MeterInterface; @@ -75,25 +77,31 @@ public function __construct( $this->writer = $writer; } - public function createCounter(string $name, ?string $unit = null, ?string $description = null): CounterInterface + public function createCounter(string $name, ?string $unit = null, ?string $description = null, array $advisory = []): CounterInterface { [$instrument, $referenceCounter] = $this->createSynchronousWriter( InstrumentType::COUNTER, $name, $unit, $description, + $advisory, ); return new Counter($this->writer, $instrument, $referenceCounter); } - public function createObservableCounter(string $name, ?string $unit = null, ?string $description = null, callable ...$callbacks): ObservableCounterInterface + public function createObservableCounter(string $name, ?string $unit = null, ?string $description = null, $advisory = [], callable ...$callbacks): ObservableCounterInterface { + if (is_callable($advisory)) { + array_unshift($callbacks, $advisory); + $advisory = []; + } [$instrument, $referenceCounter, $destructors] = $this->createAsynchronousObserver( InstrumentType::ASYNCHRONOUS_COUNTER, $name, $unit, $description, + $advisory, ); foreach ($callbacks as $callback) { @@ -104,25 +112,31 @@ public function createObservableCounter(string $name, ?string $unit = null, ?str return new ObservableCounter($this->writer, $instrument, $referenceCounter, $destructors); } - public function createHistogram(string $name, ?string $unit = null, ?string $description = null): HistogramInterface + public function createHistogram(string $name, ?string $unit = null, ?string $description = null, array $advisory = []): HistogramInterface { [$instrument, $referenceCounter] = $this->createSynchronousWriter( InstrumentType::HISTOGRAM, $name, $unit, $description, + $advisory, ); return new Histogram($this->writer, $instrument, $referenceCounter); } - public function createObservableGauge(string $name, ?string $unit = null, ?string $description = null, callable ...$callbacks): ObservableGaugeInterface + public function createObservableGauge(string $name, ?string $unit = null, ?string $description = null, $advisory = [], callable ...$callbacks): ObservableGaugeInterface { + if (is_callable($advisory)) { + array_unshift($callbacks, $advisory); + $advisory = []; + } [$instrument, $referenceCounter, $destructors] = $this->createAsynchronousObserver( InstrumentType::ASYNCHRONOUS_GAUGE, $name, $unit, $description, + $advisory, ); foreach ($callbacks as $callback) { @@ -133,25 +147,31 @@ public function createObservableGauge(string $name, ?string $unit = null, ?strin return new ObservableGauge($this->writer, $instrument, $referenceCounter, $destructors); } - public function createUpDownCounter(string $name, ?string $unit = null, ?string $description = null): UpDownCounterInterface + public function createUpDownCounter(string $name, ?string $unit = null, ?string $description = null, array $advisory = []): UpDownCounterInterface { [$instrument, $referenceCounter] = $this->createSynchronousWriter( InstrumentType::UP_DOWN_COUNTER, $name, $unit, $description, + $advisory, ); return new UpDownCounter($this->writer, $instrument, $referenceCounter); } - public function createObservableUpDownCounter(string $name, ?string $unit = null, ?string $description = null, callable ...$callbacks): ObservableUpDownCounterInterface + public function createObservableUpDownCounter(string $name, ?string $unit = null, ?string $description = null, $advisory = [], callable ...$callbacks): ObservableUpDownCounterInterface { + if (is_callable($advisory)) { + array_unshift($callbacks, $advisory); + $advisory = []; + } [$instrument, $referenceCounter, $destructors] = $this->createAsynchronousObserver( InstrumentType::ASYNCHRONOUS_UP_DOWN_COUNTER, $name, $unit, $description, + $advisory, ); foreach ($callbacks as $callback) { @@ -166,9 +186,9 @@ public function createObservableUpDownCounter(string $name, ?string $unit = null * @param string|InstrumentType $instrumentType * @return array{Instrument, ReferenceCounterInterface} */ - private function createSynchronousWriter($instrumentType, string $name, ?string $unit, ?string $description): array + private function createSynchronousWriter($instrumentType, string $name, ?string $unit, ?string $description, array $advisory = []): array { - $instrument = new Instrument($instrumentType, $name, $unit, $description); + $instrument = new Instrument($instrumentType, $name, $unit, $description, $advisory); $instrumentationScopeId = $this->instrumentationScopeId($this->instrumentationScope); $instrumentId = $this->instrumentId($instrument); @@ -213,9 +233,9 @@ private function createSynchronousWriter($instrumentType, string $name, ?string * @param string|InstrumentType $instrumentType * @return array{Instrument, ReferenceCounterInterface, ArrayAccess} */ - private function createAsynchronousObserver($instrumentType, string $name, ?string $unit, ?string $description): array + private function createAsynchronousObserver($instrumentType, string $name, ?string $unit, ?string $description, array $advisory): array { - $instrument = new Instrument($instrumentType, $name, $unit, $description); + $instrument = new Instrument($instrumentType, $name, $unit, $description, $advisory); $instrumentationScopeId = $this->instrumentationScopeId($this->instrumentationScope); $instrumentId = $this->instrumentId($instrument); @@ -293,7 +313,8 @@ private function viewRegistrationRequests(Instrument $instrument, StalenessHandl $view->unit, $view->description, $view->attributeKeys, - $metricRegistry->defaultAggregation($instrument->type), + /** @phan-suppress-next-line PhanParamTooMany @phpstan-ignore-next-line */ + $metricRegistry->defaultAggregation($instrument->type, $instrument->advisory), ), new RegistryRegistration($metricRegistry, $stalenessHandler), ]; @@ -309,6 +330,11 @@ private function instrumentationScopeId(InstrumentationScopeInterface $instrumen private function instrumentId(Instrument $instrument): string { - return serialize($instrument); + return serialize([ + $instrument->type, + $instrument->name, + $instrument->unit, + $instrument->description, + ]); } } diff --git a/src/SDK/Metrics/MetricReader/ExportingReader.php b/src/SDK/Metrics/MetricReader/ExportingReader.php index 3c2eff9f1..3d5e8f1e7 100644 --- a/src/SDK/Metrics/MetricReader/ExportingReader.php +++ b/src/SDK/Metrics/MetricReader/ExportingReader.php @@ -41,13 +41,14 @@ public function __construct(MetricExporterInterface $exporter) $this->exporter = $exporter; } - public function defaultAggregation($instrumentType): ?AggregationInterface + public function defaultAggregation($instrumentType, array $advisory = []): ?AggregationInterface { if ($this->exporter instanceof DefaultAggregationProviderInterface) { - return $this->exporter->defaultAggregation($instrumentType); + /** @phan-suppress-next-line PhanParamTooMany @phpstan-ignore-next-line */ + return $this->exporter->defaultAggregation($instrumentType, $advisory); } - return $this->_defaultAggregation($instrumentType); + return $this->_defaultAggregation($instrumentType, $advisory); } public function add(MetricSourceProviderInterface $provider, MetricMetadataInterface $metadata, StalenessHandlerInterface $stalenessHandler): void diff --git a/tests/Unit/SDK/Metrics/MeterTest.php b/tests/Unit/SDK/Metrics/MeterTest.php index 7fcd2c744..c6cb66f37 100644 --- a/tests/Unit/SDK/Metrics/MeterTest.php +++ b/tests/Unit/SDK/Metrics/MeterTest.php @@ -69,6 +69,37 @@ public function test_create_histogram(): void $meter->createHistogram('name', 'unit', 'description'); } + public function test_create_histogram_advisory(): void + { + $metricFactory = $this->createMock(MetricFactoryInterface::class); + $metricFactory->expects($this->once())->method('createSynchronousWriter') + ->with( + $this->anything(), + $this->anything(), + new InstrumentationScope('test', null, null, Attributes::create([])), + new Instrument( + InstrumentType::HISTOGRAM, + 'http.server.duration', + 's', + 'Measures the duration of inbound HTTP requests.', + ['ExplicitBucketBoundaries' => [0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10]], + ), + $this->anything(), + $this->anything(), + $this->anything(), + ) + ->willReturn([]); + + $meterProvider = $this->createMeterProviderForMetricFactory($metricFactory); + $meter = $meterProvider->getMeter('test'); + $meter->createHistogram( + 'http.server.duration', + 's', + 'Measures the duration of inbound HTTP requests.', + ['ExplicitBucketBoundaries' => [0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10]], + ); + } + public function test_create_up_down_counter(): void { $metricFactory = $this->createMock(MetricFactoryInterface::class); diff --git a/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php b/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php index 99787c303..a0997845c 100644 --- a/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php +++ b/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php @@ -53,6 +53,17 @@ public function test_default_aggregation_returns_default_aggregation(): void $this->assertEquals(new LastValueAggregation(), $reader->defaultAggregation(InstrumentType::ASYNCHRONOUS_GAUGE)); } + public function test_default_aggregation_returns_histogram_with_advisory_buckets(): void + { + $exporter = new InMemoryExporter(); + $reader = new ExportingReader($exporter); + + $this->assertEquals( + new ExplicitBucketHistogramAggregation([0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10]), + $reader->defaultAggregation(InstrumentType::HISTOGRAM, ['ExplicitBucketBoundaries' => [0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10]]), + ); + } + public function test_default_aggregation_returns_exporter_aggregation_if_default_aggregation_provider(): void { $exporter = $this->createMock(DefaultAggregationProviderExporterInterface::class);