Add Meter SDK Class#212
Conversation
-Create Synchronous and Asynchronous Instruments -Collect Synchronous and Asynchronous Insturments -Collect Synchronous and Asynchronous Instruments after their returned shared_ptr is deleted -RecordBatch for all 4 primitive types -String utility functions, IsValid() and NameAlreadyExists()
Metric instruments are collected from in the reverse order that they were created. The test assumed that they were collected in-order. This has been rectified by making all ASSERTs in the RecordBatch test examine the first aggregator in the vector of records returned from m->collect()
| { | ||
| continue; | ||
| } | ||
| auto cast_ptr = nostd::dynamic_pointer_cast<SynchronousInstrument<short>>(pair.second); |
There was a problem hiding this comment.
This specifically is where the problem lays. I need nostd::dynamic_pointer_cast to be implemented for nostd::shared_ptr. I cannot simply cast to a raw pointer as we are editing memory that the user has a shared_ptr to and this could easily lead to segfaults or undefined behavior.
I have tested my code with std::shared_ptr replacing all nostd::shared_ptrs in this class and all tests pass.
If someone could possibly look into implementing this for nostd::shared_ptr or point me in the right direction to add this functionality myself I would greatly appreciate it. Thanks!
|
This PR is ready to be reviewed again now that the issue with nostd::shared_ptr has been resolved. |
|
@Brandon-Kimberly would you rebase and resolve the conflicts? |
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
==========================================
- Coverage 93.70% 92.39% -1.32%
==========================================
Files 135 140 +5
Lines 5388 6088 +700
==========================================
+ Hits 5049 5625 +576
- Misses 339 463 +124
|
|
@reyang This PR has been rebased and all CI checks pass. I believe it is ready to merge pending review. :) |
reyang
added
the
pr:please-merge
…-20250127.x Update dependency abseil-cpp to v20250127.1
2 participants
This PR adds the header and implementation files for the SDK Meter class as well as a suite of unit tests. This class functions as the accumulator in the SDK spec. This class provides the user a way to create new metric instruments, and collect those instruments’ updates for exporting as well as a facility for recording to a batch of instruments in a single API call.
IMPORTANT NOTES:
Issue
The problem I am running into currently is that, I need to cast from an API component to an SDK component. This is because all functions and variables related to the collection of metrics are, according to the spec, only defined in the SDK. This means that we cannot have virtual functions in the API to access the information we need to collect and send for export such as the instruments’ aggregators.
The issue with that is, nostd::shared_ptr does not work with std::dynamic_pointer_cast. We cannot simply cast to a raw pointer as this could cause memory leaks or segfaults should we try to access memory that was freed by the user’s shared_ptr to the instrument. We need to use nostd::shared_ptr because the pointers to the created instruments that are returned to the user, must also be placed into a map so that their measurements can be collected later. The pointer in the map must be deleted when the user’s pointer goes out of scope or it will segfault on the next call to Collect(). I am unsure about how to implement this with raw pointers.
I believe the cleanest solution to this problem could be to implement dynamic_pointer_cast for nostd::shared_ptr. I have tested my class with std::shared_ptr and std::dynamic_pointer_cast and all functionality works as intended. In order to provide this functionality to nostd::shared_ptr I believe we would need to add an aliasing constructor to the class. I have looked into this a bit but I am out of my depth with STL code.
Any help or advice on how to fix this problem would be greatly appreciated. I am currently blocked on this problem and it is preventing me from being able to complete contributing a fully functional metrics API and SDK.
UPDATE
Thanks to help from @pyohannes I was able to resolve the issue involving dynamic_pointer_cast. This PR is still dependent on code in PR #191 and PR #162 and should not be merged before them but the code is complete and awaiting more reviews.