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[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: 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%)	:arrow_up:

... and 1 file with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: 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.

[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.