Metrics API : Provider , MeterProvider, Meter, SynchronousInstrument

[lalitb]

Fixes # (issue)

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #1033 (5736834) into main (3807726) will decrease coverage by 0.44%.
The diff coverage is 69.82%.

@@            Coverage Diff             @@
##             main    #1033      +/-   ##
==========================================
- Coverage   94.85%   94.41%   -0.43%     
==========================================
  Files         151      158       +7     
  Lines        5971     6077     +106     
==========================================
+ Hits         5663     5737      +74     
- Misses        308      340      +32     

Impacted Files	Coverage Δ
api/include/opentelemetry/metrics/noop.h	28.89% <28.89%> (ø)
api/include/opentelemetry/metrics/meter.h	100.00% <100.00%> (ø)
api/include/opentelemetry/metrics/meter_provider.h	100.00% <100.00%> (ø)
api/include/opentelemetry/metrics/provider.h	100.00% <100.00%> (ø)
...i/include/opentelemetry/metrics/sync_instruments.h	100.00% <100.00%> (ø)
api/test/metrics/meter_provider_test.cc	100.00% <100.00%> (ø)
api/test/metrics/noop_sync_instrument_test.cc	100.00% <100.00%> (ø)

[jsuereth]

Nit: You may want to have a variant of this that takes a void* state parameter and passes that to the callback (or hide that with templates).

I understand you need this API to be bin-compat so likely std::function is out. However, it's entirely likely these observables need to track their own internal state.

[lalitb]

Does it mean having these two variants:
CreateFloatObservableCounter( .., .., .., callback ) // existing
and
CreateFloatObservableCounter(.., .., .., callback, void *)

Passing state through lambda/std::function would be the best way, but you are correct on issues with ABI-compat. I initially thought this was in the scope of SDK implementation ( as ObserverResult implementation is provided by SDK , and state can be passed through its constructor) but let me give it a thought. Probably we can add it separately from this PR if needed?

[lalitb]

Have modified the callback to contain the state. Passing void * or va_args has potential danger of run-time crash, so used KeyValueIterable for it as below:
void (*callback)(ObserverResult<double> &result, const common::KeyValueIterable &state)

[jsuereth]

I'm not sure this is what I meant.

Specifically, what you really want is to pass an instance of a class and its state, vs. a key-value iterable (which is immutable and you cannot change its state).

Maybe try an implementation of one of these methods against some kind of stateful backend where you pull values and see what works best.

I agree void* is unsafe, I'm not sure of a bin-compat alternative....

[jsuereth]

I did some of my own research and likely my understanding of best-practices around callbacks is off in C++. Stateful callbacks apparently commonly use a lambda to capture a local variable that contains state, and likely works here.

Specifically, just want to make sure you can do something like the following:

const MyCustomMetrics& LatestMetrics() {
   static std::unique_ptr<Storage> lastMetricPull;
   static long lastMetricPullTime = 0;
   if (lastMetricPullTime < someThreasholdAgainstCurrentTime) {
      delete lastMetricPull;
      lastMetricPull = expensiveCallForMetrics();
   }
}


void someNormalMethod() {
   meterProivder->CreateFloatObservable(..., ..., ..., [&](ObservableMeasuremnet m) {
      m.observe( LatestMetrics().someValue);
  });
}

I.e. as long as we have a mechanism to store long-running state in "raw" form, we're fine. I forgot lambdas do all sorts of things for you know, so I think your original signature was fine, sorry for the noise!

[jsuereth]

Nit: You need a variant of this method (in every synchronous instrument) that also takes "Context".

By default these value/attributes need to pull Context.current or the C++ equivalent.

[lalitb]

Do we need to support explicit context at the instrumentation/api level? I was thinking of these measurements to be associated with the current context. As it's not there in specs too ( https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#record )

[jsuereth]

It's up to you. If you provide any explicit context-api, then I'd say you should also allow it here (we do in java).

The key is that context CAN be used in measurements to do exemplar sampling.

[jsuereth]

Great progress @lalitb! Excited to see this!!!!

[jsuereth]

Looks good!