Add Asynchronous Metric Instruments SDK #191
Conversation
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
=======================================
Coverage 93.63% 93.63%
=======================================
Files 134 134
Lines 5345 5345
=======================================
Hits 5005 5005
Misses 340 340 |
| } | ||
|
|
||
| /* | ||
| * Activate the intsrument's callback function to record a measurement. This |
There was a problem hiding this comment.
| * Activate the intsrument's callback function to record a measurement. This | |
| * Activate the instrument's callback function to record a measurement. This |
| #include <map> | ||
| #include <sstream> | ||
| #include <vector> | ||
| #include <memory> |
There was a problem hiding this comment.
Minor: would you sort includes alphabetically unless there is a good reason not to do so?
There was a problem hiding this comment.
The format tools reorder the includes but I sorted them where I could.
| } | ||
|
|
||
| // Public mapping from labels (stored as strings) to their respective aggregators | ||
| std::unordered_map<std::string, std::shared_ptr<Aggregator<T>>> boundAggregators_; |
There was a problem hiding this comment.
There seems to be indentation issue.
| else | ||
| { | ||
| if (value < 0){ | ||
| throw std::invalid_argument("Counter instrument updates must be non-negative."); |
There was a problem hiding this comment.
@pyohannes do you think we should check __EXCEPTIONS and call std::terminate?
I think we shouldn't throw if exception is disabled, however we don't want to kill the process as this is not a critical failure.
There was a problem hiding this comment.
I've added a check and call to terminate :)
| return ret; | ||
| } | ||
|
|
||
| // Public mapping from labels (stored as strings) to their respective aggregators |
There was a problem hiding this comment.
This seems to be another indentation issue, would you rebase and check if the formatter CI would pass?
ankit-bhargava
force-pushed
the
metrics-sdk-asyncinstr
branch
from
d608f83 to
6e4cb4d
Compare
|
@reyang Thanks for the review! I rebased and addressed your comments. Additionally, we discovered a bug related to the way we were dealing with concurrency in our synchronous instruments. I added fixes in this PR out of convenience (you can see the changes in the |
ankit-bhargava
force-pushed
the
metrics-sdk-asyncinstr
branch
from
e984a9c to
7b76bc4
Compare
|
@reyang This should be ready to merge unless there are any further concerns |
This PR implements the asynchronous instrument classes described in the Metric Instruments API (PR #161). These classes are capable of actually collecting telemetry data unlike no-op implementations included with the API. Instruments employ Aggregators (pending approval in PR #178 and PR #181 ) to combine the updates they receive into meaningful values.
A test suite which validates each instrument’s ability to record values in single-threaded and concurrent scenarios is included. Functions necessary for the metrics pipeline as a whole such as reference counting are tested as well.
This PR in conjunction with (PR #179) provides support for all metric instruments listed in the spec. This PR will not compile without all the other PRs mentioned in this description.
Note: We templated instruments to support a wider array of scalar types. Templating prevents us from using out of line declarations as we normally would. As a result, we left all implementation code in the header file. If there are any suggestions to remedy this we will gladly make the change. Code for the base classes which asynchronous instruments inherit from is in PR #161.