add validation in MetricCollector constructor and enhance related tests

[Reneg973]

Fixes # (issue)
an access violation in constructor of MetricProducer when giving nullptr metric_reader.

Changes

Need clarification first:
Is it allowed to throw exceptions in constructor at least? Currently I see a lot of classes allowing nullptr without checking early. This leads to objects having an invalid state, while member functions do nothing except spamming the log, that a required member is not set correctly.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.02%. Comparing base (d882c6b) to head (9f80e3d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3724      +/-   ##
==========================================
+ Coverage   90.00%   90.02%   +0.02%     
==========================================
  Files         225      225              
  Lines        7099     7102       +3     
==========================================
+ Hits         6389     6393       +4     
+ Misses        710      709       -1     

Files with missing lines	Coverage Δ
sdk/src/metrics/state/metric_collector.cc	96.78% <100.00%> (+3.56%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Reneg973]

OK, I understand you disabled exceptions. But, there is a lot of functions which may throw. Some of them even have noexcept, means it simply crashes.
Note that, under circumstances, every new could throw, every string or map or vector creation. Also, many parameters are not checked for nullptr and simply accessed. How did you plan to handle all this missing checks?
You also would need to have a custom new handler which doesn't throw. Possible, but a lot of work.

The first thing missing, if no function/even no constructor must throw, to add noexcept everywhere.

[Copilot]

Pull Request Overview

This PR improves error handling and robustness in the MetricCollector class by adding validation for null pointers and exception handling. The constructor now validates input parameters, and the Produce() method wraps metric collection in try-catch blocks to handle exceptions gracefully.

Key changes:

• Added null checks for context and metric_reader parameters in constructor and methods
• Added exception handling in Produce() method's ForEachMeter callback
• Modified test helper to return shared_ptr instead of weak_ptr for simpler test code
• Added new test case to verify invalid argument handling

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
sdk/src/metrics/state/metric_collector.cc	Added null pointer validation in constructor, GetAggregationTemporality, ForceFlush, and Shutdown; added try-catch blocks in Produce() method
sdk/include/opentelemetry/sdk/metrics/state/metric_collector.h	Marked constructor as noexcept
sdk/test/metrics/metric_collector_test.cc	Changed test helper return type from weak_ptr to shared_ptr, removed noexcept from helper, added new test for invalid arguments, updated all test usages

[Copilot]

Missing space between the error message and the stream insertion. Should be: \"Error constructing MetricCollector. \" (with trailing space) for proper formatting.

Suggested change

	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::MetricCollector] - Error constructing MetricCollector."
	<< "The context is invalid");
	if (metric_reader_)
	metric_reader_->SetMetricProducer(this);
	else
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::MetricCollector] - Error constructing MetricCollector."
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::MetricCollector] - Error constructing MetricCollector. "
	<< "The context is invalid");
	if (metric_reader_)
	metric_reader_->SetMetricProducer(this);
	else
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::MetricCollector] - Error constructing MetricCollector. "

[Copilot]

Missing space between the error message and the stream insertion. Should be: \"Error constructing MetricCollector. \" (with trailing space) for proper formatting.

Suggested change

	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::MetricCollector] - Error constructing MetricCollector."
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::MetricCollector] - Error constructing MetricCollector. "

[Copilot]

Missing space between the error message and the stream insertion. Should be: \"Error getting aggregation temporality. \" (with trailing space) for proper formatting.

Suggested change

	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::GetAggregationTemporality] - Error getting aggregation temporality."
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::GetAggregationTemporality] - Error getting aggregation temporality. "

[Copilot]

Missing space between the error message and the stream insertion. Should be: \"Error during collecting. \" (with trailing space) for proper formatting.

Suggested change

	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::Collect] - Error during collecting."
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::Collect] - Error during collecting. "

[Copilot]

Missing space between the error message and the stream insertion. Should be: \"Error during collecting. \" (with trailing space) for proper formatting.

Suggested change

	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::Collect] - Error during collecting."
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::Collect] - Error during collecting. "

[Copilot]

Missing space between the error message and the stream insertion. Should be: \"Error accessing meter_reader_. \" (with trailing space) for proper formatting.

Suggested change

	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::ForceFlush] - Error accessing meter_reader_."
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::ForceFlush] - Error accessing meter_reader_. "

[Copilot]

Missing space between the error message and the stream insertion. Should be: \"Error accessing meter_reader_. \" (with trailing space) for proper formatting.

Suggested change

	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::Shutdown] - Error accessing meter_reader_."
	OTEL_INTERNAL_LOG_ERROR("[MetricCollector::Shutdown] - Error accessing meter_reader_. "

[lalitb]

Is it allowed to throw exceptions in constructor at least? Currently I see a lot of classes allowing nullptr without checking early. This leads to objects having an invalid state, while member functions do nothing except spamming the log, that a required member is not set correctly.

Public-facing APIs: Return real or no-op objects, shouldn't be nullptr ideally.
Internal SDK code: Assumes proper dependency injection, and nullptr shouldn't be passed in constructors. Maybe we can validate this by asserting in debug build.

But, exceptions are not thrown.

p.s btw sorry for adding copilot for review. didn't realize it's still draft.

[Reneg973]

Public-facing APIs: Return real or no-op objects, shouldn't be nullptr ideally.
and nullptr shouldn't be passed in constructors

Can you ensure that? E.g.

auto meter = prov->CreateMeter(...);
...->AddMeter(std::move(meter));

Where should the validation be? CreateMeter() could return the NoopMeter in case new fails. Should that be done? And AddMeter() could get a nullptr, aeven in Release, a check just in Debug won't be enough. Currently its not checked on output nor input.
And we don't program in an ideal system where new never fails. The chance may be low, but never 0.

Proposals to make the code safer:

• use of non-throwing new
• provide nostd::make_unique and nostd::make_shared using non-throwing new
• try/catch everything using dynamic allocations (vector/string/map...)
• provide a helper member function bool IsValid() const noexcept on every object which can have invalid state.