Repository: open-telemetry/opentelemetry-php (php)

Issue:
MetricExporterInterface.temporality(metric) is not part of the spec
From https://github.com/open-telemetry/community/issues/1536#issuecomment-1621988512

> MetricExporterInterface.temporality(metric) is not part of the spec https://github.com/open-telemetry/opentelemetry-php/blob/cc92a07eac5294055b0d477fb7e47d6480381505/src/SDK/Metrics/MetricExporterInterface.php#L21


Pull request:
move temporality into own interface
per TC review, temporality is not part of MetricExporterInterface. Riffing on Java's solution, this moves `temporality` into `AggregationTemporalitySelectorInterface`, and adds that interface to MetricExporter instances that use it (everything except `NoopMetricExporter`).

Closes #1087 

Patch:
diff --git a/src/Contrib/Otlp/MetricExporter.php b/src/Contrib/Otlp/MetricExporter.php
index d2ca9c27..93ec5c48 100644
--- a/src/Contrib/Otlp/MetricExporter.php
+++ b/src/Contrib/Otlp/MetricExporter.php
@@ -7,6 +7,7 @@ namespace OpenTelemetry\Contrib\Otlp;
 use OpenTelemetry\API\Behavior\LogsMessagesTrait;
 use Opentelemetry\Proto\Collector\Metrics\V1\ExportMetricsServiceResponse;
 use OpenTelemetry\SDK\Common\Export\TransportInterface;
+use OpenTelemetry\SDK\Metrics\AggregationTemporalitySelectorInterface;
 use OpenTelemetry\SDK\Metrics\Data\Temporality;
 use OpenTelemetry\SDK\Metrics\MetricMetadataInterface;
 use OpenTelemetry\SDK\Metrics\PushMetricExporterInterface;
@@ -18,7 +19,7 @@ use Throwable;
  * @see https://github.com/open-telemetry/opentelemetry-specification/blob/main/experimental/serialization/json.md#json-file-serialization
  * @psalm-import-type SUPPORTED_CONTENT_TYPES from ProtobufSerializer
  */
-final class MetricExporter implements PushMetricExporterInterface
+final class MetricExporter implements PushMetricExporterInterface, AggregationTemporalitySelectorInterface
 {
     use LogsMessagesTrait;
 
diff --git a/src/SDK/Metrics/AggregationTemporalitySelectorInterface.php b/src/SDK/Metrics/AggregationTemporalitySelectorInterface.php
new file mode 100644
index 00000000..f046d033
--- /dev/null
+++ b/src/SDK/Metrics/AggregationTemporalitySelectorInterface.php
@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\SDK\Metrics;
+
+use OpenTelemetry\SDK\Metrics\Data\Temporality;
+
+interface AggregationTemporalitySelectorInterface
+{
+    /**
+     * Returns the temporality to use for the given metric.
+     *
+     * It is recommended to return {@see MetricMetadataInterface::temporality()}
+     * if the exporter does not require a specific temporality.
+     *
+     * @return string|Temporality|null temporality to use, or null to signal
+     *         that the given metric should not be exported by this exporter
+     */
+    public function temporality(MetricMetadataInterface $metric);
+}
diff --git a/src/SDK/Metrics/MetricExporter/ConsoleMetricExporter.php b/src/SDK/Metrics/MetricExporter/ConsoleMetricExporter.php
index b9330a11..62ea7b53 100644
--- a/src/SDK/Metrics/MetricExporter/ConsoleMetricExporter.php
+++ b/src/SDK/Metrics/MetricExporter/ConsoleMetricExporter.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace OpenTelemetry\SDK\Metrics\MetricExporter;
 
 use OpenTelemetry\SDK\Common\Instrumentation\InstrumentationScopeInterface;
+use OpenTelemetry\SDK\Metrics\AggregationTemporalitySelectorInterface;
 use OpenTelemetry\SDK\Metrics\Data\Metric;
 use OpenTelemetry\SDK\Metrics\Data\Temporality;
 use OpenTelemetry\SDK\Metrics\MetricMetadataInterface;
@@ -15,7 +16,7 @@ use OpenTelemetry\SDK\Resource\ResourceInfo;
  * Console metrics exporter.
  * Note that the output is human-readable JSON, not compatible with OTLP.
  */
-class ConsoleMetricExporter implements PushMetricExporterInterface
+class ConsoleMetricExporter implements PushMetricExporterInterface, AggregationTemporalitySelectorInterface
 {
     /**
      * @var string|Temporality|null
diff --git a/src/SDK/Metrics/MetricExporter/InMemoryExporter.php b/src/SDK/Metrics/MetricExporter/InMemoryExporter.php
index 208e956f..6bbab8b7 100644
--- a/src/SDK/Metrics/MetricExporter/InMemoryExporter.php
+++ b/src/SDK/Metrics/MetricExporter/InMemoryExporter.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace OpenTelemetry\SDK\Metrics\MetricExporter;
 
 use function array_push;
+use OpenTelemetry\SDK\Metrics\AggregationTemporalitySelectorInterface;
 use OpenTelemetry\SDK\Metrics\Data\Metric;
 use OpenTelemetry\SDK\Metrics\Data\Temporality;
 use OpenTelemetry\SDK\Metrics\MetricExporterInterface;
@@ -13,7 +14,7 @@ use OpenTelemetry\SDK\Metrics\MetricMetadataInterface;
 /**
  * @see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/in-memory.md
  */
-final class InMemoryExporter implements MetricExporterInterface
+final class InMemoryExporter implements MetricExporterInterface, AggregationTemporalitySelectorInterface
 {
     /**
      * @var list<Metric>
diff --git a/src/SDK/Metrics/MetricExporter/NoopMetricExporter.php b/src/SDK/Metrics/MetricExporter/NoopMetricExporter.php
index 4d3c070a..0cac12fa 100644
--- a/src/SDK/Metrics/MetricExporter/NoopMetricExporter.php
+++ b/src/SDK/Metrics/MetricExporter/NoopMetricExporter.php
@@ -5,19 +5,9 @@ declare(strict_types=1);
 namespace OpenTelemetry\SDK\Metrics\MetricExporter;
 
 use OpenTelemetry\SDK\Metrics\MetricExporterInterface;
-use OpenTelemetry\SDK\Metrics\MetricMetadataInterface;
 
 class NoopMetricExporter implements MetricExporterInterface
 {
-
-    /**
-     * @inheritDoc
-     */
-    public function temporality(MetricMetadataInterface $metric)
-    {
-        return $metric->temporality();
-    }
-
     /**
      * @inheritDoc
      */
diff --git a/src/SDK/Metrics/MetricExporterInterface.php b/src/SDK/Metrics/MetricExporterInterface.php
index f5964a9f..fa47fbf9 100644
--- a/src/SDK/Metrics/MetricExporterInterface.php
+++ b/src/SDK/Metrics/MetricExporterInterface.php
@@ -5,21 +5,9 @@ declare(strict_types=1);
 namespace OpenTelemetry\SDK\Metrics;
 
 use OpenTelemetry\SDK\Metrics\Data\Metric;
-use OpenTelemetry\SDK\Metrics\Data\Temporality;
 
 interface MetricExporterInterface
 {
-    /**
-     * Returns the temporality to use for the given metric.
-     *
-     * It is recommended to return {@see MetricMetadataInterface::temporality()}
-     * if the exporter does not require a specific temporality.
-     *
-     * @return string|Temporality|null temporality to use, or null to signal
-     *         that the given metric should not be exported by this exporter
-     */
-    public function temporality(MetricMetadataInterface $metric);
-
     /**
      * @param iterable<int, Metric> $batch
      */
diff --git a/src/SDK/Metrics/MetricReader/ExportingReader.php b/src/SDK/Metrics/MetricReader/ExportingReader.php
index 561cfd40..3c2eff9f 100644
--- a/src/SDK/Metrics/MetricReader/ExportingReader.php
+++ b/src/SDK/Metrics/MetricReader/ExportingReader.php
@@ -6,6 +6,7 @@ namespace OpenTelemetry\SDK\Metrics\MetricReader;
 
 use function array_keys;
 use OpenTelemetry\SDK\Metrics\AggregationInterface;
+use OpenTelemetry\SDK\Metrics\AggregationTemporalitySelectorInterface;
 use OpenTelemetry\SDK\Metrics\DefaultAggregationProviderInterface;
 use OpenTelemetry\SDK\Metrics\DefaultAggregationProviderTrait;
 use OpenTelemetry\SDK\Metrics\MetricExporterInterface;
@@ -54,6 +55,9 @@ final class ExportingReader implements MetricReaderInterface, MetricSourceRegist
         if ($this->closed) {
             return;
         }
+        if (!$this->exporter instanceof AggregationTemporalitySelectorInterface) {
+            return;
+        }
         if (!$temporality = $this->exporter->temporality($metadata)) {
             return;
         }


Test patch:
diff --git a/tests/Unit/Contrib/Otlp/MetricExporterFactoryTest.php b/tests/Unit/Contrib/Otlp/MetricExporterFactoryTest.php
index 44846cf1..1e12e4d0 100644
--- a/tests/Unit/Contrib/Otlp/MetricExporterFactoryTest.php
+++ b/tests/Unit/Contrib/Otlp/MetricExporterFactoryTest.php
@@ -10,6 +10,7 @@ use OpenTelemetry\SDK\Common\Configuration\KnownValues;
 use OpenTelemetry\SDK\Common\Configuration\Variables;
 use OpenTelemetry\SDK\Common\Export\TransportFactoryInterface;
 use OpenTelemetry\SDK\Common\Export\TransportInterface;
+use OpenTelemetry\SDK\Metrics\AggregationTemporalitySelectorInterface;
 use OpenTelemetry\SDK\Metrics\Data\Temporality;
 use OpenTelemetry\SDK\Metrics\MetricMetadataInterface;
 use PHPUnit\Framework\TestCase;
@@ -59,6 +60,7 @@ class MetricExporterFactoryTest extends TestCase
         $factory = new MetricExporterFactory($this->transportFactory);
         $exporter = $factory->create();
 
+        $this->assertInstanceOf(AggregationTemporalitySelectorInterface::class, $exporter);
         $this->assertSame($expected, $exporter->temporality($this->createMock(MetricMetadataInterface::class)));
     }
 
diff --git a/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php b/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php
index 061e45e6..99787c30 100644
--- a/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php
+++ b/tests/Unit/SDK/Metrics/MetricReader/ExportingReaderTest.php
@@ -8,6 +8,7 @@ use OpenTelemetry\SDK\Common\Instrumentation\InstrumentationScopeInterface;
 use OpenTelemetry\SDK\Metrics\Aggregation\ExplicitBucketHistogramAggregation;
 use OpenTelemetry\SDK\Metrics\Aggregation\LastValueAggregation;
 use OpenTelemetry\SDK\Metrics\Aggregation\SumAggregation;
+use OpenTelemetry\SDK\Metrics\AggregationTemporalitySelectorInterface;
 use OpenTelemetry\SDK\Metrics\Data\DataInterface;
 use OpenTelemetry\SDK\Metrics\Data\Metric;
 use OpenTelemetry\SDK\Metrics\Data\Temporality;
@@ -78,7 +79,6 @@ final class ExportingReaderTest extends TestCase
     public function test_add_does_not_create_metric_source_if_exporter_temporality_null(): void
     {
         $exporter = $this->createMock(MetricExporterInterface::class);
-        $exporter->method('temporality')->willReturn(null);
         $reader = new ExportingReader($exporter);
 
         $provider = $this->createMock(MetricSourceProviderInterface::class);
@@ -168,7 +168,7 @@ final class ExportingReaderTest extends TestCase
 
     public function test_shutdown_exports_metrics(): void
     {
-        $exporter = $this->createMock(MetricExporterInterface::class);
+        $exporter = $this->createMock(MetricExporterWithTemporalityInterface::class);
         $provider = $this->createMock(MetricSourceProviderInterface::class);
         $source = $this->createMock(MetricSourceInterface::class);
         $source->method('collect')->willReturn($this->createMock(Metric::class));
@@ -224,3 +224,7 @@ final class ExportingReaderTest extends TestCase
 interface DefaultAggregationProviderExporterInterface extends MetricExporterInterface, DefaultAggregationProviderInterface
 {
 }
+
+interface MetricExporterWithTemporalityInterface extends MetricExporterInterface, AggregationTemporalitySelectorInterface
+{
+}