Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Metrics SDK] Add support for Pull Metric Exporter #1561

Closed
sam-leitch-oxb opened this issue Aug 11, 2022 · 1 comment · Fixed by #1701
Closed

[Metrics SDK] Add support for Pull Metric Exporter #1561

sam-leitch-oxb opened this issue Aug 11, 2022 · 1 comment · Fixed by #1701
Assignees
Labels
metrics priority:p1 Issues that are blocking spec-compliance Not compliant to OpenTelemetry specs

Comments

@sam-leitch-oxb
Copy link

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.

@lalitb lalitb added this to the Metrics v1.0.0 (GA) milestone Aug 11, 2022
@lalitb
Copy link
Member

lalitb commented Aug 12, 2022

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 lalitb added metrics spec-compliance Not compliant to OpenTelemetry specs labels Aug 12, 2022
@lalitb lalitb added the priority:p1 Issues that are blocking label Aug 24, 2022
@lalitb lalitb changed the title Add support for Pull Metric Exporter [Metrics SDK] Add support for Pull Metric Exporter Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics priority:p1 Issues that are blocking spec-compliance Not compliant to OpenTelemetry specs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants