Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 API : Provider , MeterProvider, Meter, SynchronousInstrument #1033
Metrics API : Provider , MeterProvider, Meter, SynchronousInstrument #1033
Changes from 4 commits
092339f
e8674e2
4a829a0
d0d6131
751b44d
7f47b1e
49ff6e2
0fd654b
ff80cbe
67961a4
5b2e1cf
35e71e7
bacbcf6
b745284
b087041
93341e2
9ddac95
96ee875
d17ea27
5736834
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean having these two variants:
CreateFloatObservableCounter( .., .., .., callback )
// existingand
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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!