Add Metric Instruments API#161
Conversation
Codecov Report
@@ Coverage Diff @@
## master #161 +/- ##
==========================================
- Coverage 95.68% 92.30% -3.39%
==========================================
Files 96 101 +5
Lines 2877 3169 +292
==========================================
+ Hits 2753 2925 +172
- Misses 124 244 +120
|
lykkin
left a comment
lykkin
left a comment
There was a problem hiding this comment.
Looks solid! Mostly type related questions. Will revisit again when I have more time.
| virtual void observe(int value, const trace::KeyValueIterable &labels) {} | ||
| }; | ||
|
|
||
| class DoubleValueObserver : public AsynchronousInstrument |
There was a problem hiding this comment.
It looks like there are a lot of these that only differ based on the type of value that is accepted by the observe method. Could this be generalized through templating or macros?
I'm also fine leaving as is and updating if/when there is reason to (e.g reduction of duplicated methods added to each of the instruments).
| * @param value is the numerical representation of the metric being captured | ||
| * @return void | ||
| */ | ||
| virtual void update(nostd::variant<int, double> value) final {} |
There was a problem hiding this comment.
I don't see a situation where you would want to change the type you are calling update with after constructing the instrument, would it make sense to template the instrument class and use its type here? That way you don't have any overhead at runtime to unwrap the value.
There was a problem hiding this comment.
Our initial hesitance with templates was that they aren't restrictive enough given that we only want to support int and double types. That said, I think you make a great point and I'll reevaluate with my team as it does reduce overhead and cut down on duplicate code.
If I were to template, would I then be responsible for checking that the user picks a suitable type (perhaps by using something like <type_traits>) or expect them to use the instruments with only ints and doubles as intended?
There was a problem hiding this comment.
Using traits could be one way to go. The compiler errors for using an unsupported type would be pretty gross in this situation, I don't know how much we want to worry about that though.
Alternatively, you could break them out into their own classes with specific types they accept, like DoubleInstrument, and IntInstrument. Code duplication in that case seems pretty rough.
We definitely want to help the user out where possible by putting some guard rails up on the types they can use, one way or another.
| } | ||
|
|
||
| // Return the instrument name | ||
| virtual nostd::string_view GetName() { return nostd::string_view(""); } |
There was a problem hiding this comment.
Do we want the default implementation or this could be pure virtual?
There was a problem hiding this comment.
I'll make that change, though the tradeoff is substantial code duplication in the noop instruments further down the road.
|
|
||
| /** | ||
| * Captures data by activating the callback function associated with the | ||
| * instrument and storing its return value. Callbacks for asychronous |
There was a problem hiding this comment.
| * instrument and storing its return value. Callbacks for asychronous | |
| * instrument and storing its return value. Callbacks for asynchronous |
| }; | ||
|
|
||
| } // namespace metrics | ||
| OPENTELEMETRY_END_NAMESPACE No newline at end of file |
There was a problem hiding this comment.
nit: add a blank line before EOF.
reyang
left a comment
reyang
left a comment
There was a problem hiding this comment.
LGTM.
Two non-blocking things:
- We might want to cover
floatandshortbesidesdoubleandintfor memory efficiency consideration (e.g. especially if we have lots of dimensions). - We might need to leverage C++ template to reduce the duplicated code.
|
I will template this before we merge; it should address the major concerns raised here. |
|
@ankit-bhargava if refactoring this PR into template code takes too much time, I'm okay with merge it to unblock the other work. Please let me know. |
reyang
added
the
pr:please-merge
|
@ankit-bhargava would you rebase? Thanks. |
ankit-bhargava
changed the title
|
Rebased with some changes |
ankit-bhargava
changed the title
Update dependency gazelle to v0.42.0
3 participants
This PR includes API support for metric instruments which are used to capture metrics data from an application. All instruments descend from a base Instrument class which contains basic information such as the name and description. Base classes for bound synchronous, unbound synchronous, and asynchronous instruments specify the required methods for their respective derived types. The framework for observer callbacks which are necessary in asynchronous instrument recording is also added.
Since C++ is strongly typed, every instrument type comes in an Int and Double variant. Per the minimal implementation design guidelines, a suite of no-op instruments is provided which can be injected into code without causing any compilation or runtime errors. This has been verified through unit tests found in noop_instrument_test.cc.
Questions
Ideally, the "bind" function in the base SynchronousInstrument class would be virtual and overriden at each subsequent level below it. I was hoping to use covariant return types to accomplish this so that the bin function for an IntCounter for example could return a pointer to a BoundIntCounter while still conforming to the inheritance pattern. I was unable to do this because shared pointers are not afforded the same leniency in return types, so I would have to dynamic cast in order to achieve the same result. As such, I simply hide the function in the base class rather than overriding. If there is a better solution, please let me know.