Metrics API : Provider , MeterProvider, Meter, SynchronousInstrument #1033
Conversation
lalitb
changed the title
Codecov Report
@@ 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
|
lalitb
changed the title
| nostd::string_view description = "", | ||
| nostd::string_view unit = "", | ||
| const bool enabled = true, | ||
| void (*callback)(ObserverResult<float>)) noexcept = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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!
| * @param value The increment amount. May be positive, negative or zero. | ||
| * @param attributes A set of attributes to associate with the count. | ||
| */ | ||
| virtual void Record(T value, const common::KeyValueIterable &attributes) noexcept = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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.
|
Great progress @lalitb! Excited to see this!!!! |
lalitb
changed the title
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.mdupdated for non-trivial changes