opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

[Metrics SDK] Add support for Pull Metric Exporter

Open sam-leitch-oxb opened this issue 3 years ago • 1 comments
trafficstars

The MeterProvider::AddMetricReader interface does not make it easy to create pull-based readers. For example, my unit tests look something like this:

// Attempt at Pull-based test reader
class TestReader : public opentelemetry::v1::sdk::metrics::MetricReader {
  void OnInitialized() noexcept override {}

  bool OnForceFlush(std::chrono::microseconds timeout) noexcept override {
    return true;
  }

  bool OnShutDown(std::chrono::microseconds timeout) noexcept override {
    return true;
  }
};

BOOST_AUTO_TEST_CASE(TestMetrics) {
  opentelemetry::v1::sdk::metrics::MeterProvider meter_provider;
  auto meter = meter_provider.GetMeter("unit_test_meter", "0.0.0");
  auto reader = std::make_unique<TestReader>();
  auto* readerPtr = reader.get();  // Work-around for funky OTel SDK
  meter_provider.AddMetricReader(std::move(reader));

  // ... generate metric data ...

  readerPtr->Collect([&](auto& records) {
    BOOST_TEST(has(records, "my.test.meter", {{"att1", "value1"}}, 12345L));

    return true;
  });
}

Notice the raw pointer reference to object owned by the std::unique_ptr (yuck)

I would suggest two options for an alternative:

The simplest change would be to change the AddMetricReader method:

  void AddMetricReader(std::shared_ptr<MetricReader> reader) noexcept;

To allow for shared ownership.

A more complex (and somewhat nicer IMHO) fix would be to abstract away the MetricReader class entirely:

  void AddPeriodicPushMetricExporter(std::unique_ptr<MetricExporter> exporter, std::chrono::duration interval, std::chrono::duration timeout);

  std::unique_ptr<MetricReader> AddPullMetricExporter(std::unique_ptr<MetricExporter> exporter, std::chrono::duration timeout);

Where the returned MetricReader interface provides the Collect() method that can be called to trigger MetricExporter::Collect

Both options follow the SDK documentation as well as I understand them.

sam-leitch-oxb avatar Aug 11 '22 18:08 sam-leitch-oxb

Thanks for raising the issue. I like the second approach, though it needs some design discussion. As we are nearing the Release Candidate, have added it for the next milestone (GA).

lalitb avatar Aug 12 '22 20:08 lalitb