Create libstatistics_collector package #117
Conversation
Move relevant classes Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
- Coverage 38.77% 33.54% -5.24%
==========================================
Files 39 26 -13
Lines 1488 1106 -382
Branches 908 724 -184
==========================================
- Hits 577 371 -206
+ Misses 67 63 -4
+ Partials 844 672 -172
Continue to review full report at Codecov.
|
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Ah sorry, I marked this as ready for review, but neglected to move the topic statistics classes to the libcollector package. |
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Will look into the decreased code coverage, waiting for the latest report. |
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.
Only concern is that libcollector
already exists and does something similar:
libcollector - API for the Performance Tools collector library
The shared object, libcollector.so, is used to collect performance data and is normally loaded using LD_PRELOAD by the collect command or by dbx when the Collector is enabled.
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Ditto Zach's comment, I'm assuming this was part of the design discussion but any chance we can make the library name a bit less generic to avoid potential name clashes? |
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
It appears that we are generating reports for the system_metrics_collector package only via this report, so that would make sense why coverage decreased given the only changes are moving code. |
LIBRARY DESTINATION lib | ||
ARCHIVE DESTINATION lib | ||
RUNTIME DESTINATION bin | ||
INCLUDES DESTINATION include |
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.
isn'it the default?
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.
What about this tested on OS X and Windows? If ROS2 is depending on it, I guess we'll need to make sure it works too there?
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.
Removed ARCHIVE
and INCLUDES
. Are you saying we should remove the whole thing?
SGTM with the structure cleanup, consider using a Pimpl to avoid leaking the mutex to your public header. |
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Updated name and made sure tests are passing. Still needs the following:
|
cd31ddb
to
df3cf64
Compare
ament_target_dependencies(test_received_message_age rcl rcpputils std_msgs sensor_msgs) | ||
endif() | ||
|
||
#todo remove? |
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.
you'll need this, my comment was about removing setting ARCHIVE, etc. explicitly because I'm not sure whether it'd work on Windows.
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 think this is also the wrong syntax for a todo comment
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.
Removed ARCHIVE
and INCLUDES
(seeing as how there's a separate install()
command that follows for the include/
directory).
libstatistics_collector/src/libstatistics_collector/collector/collector.cpp
Show resolved
Hide resolved
Signed-off-by: Miaofei <miaofei@amazon.com>
libstatistics_collector/include/libstatistics_collector/collector/metric_details_interface.hpp
Outdated
Show resolved
Hide resolved
libstatistics_collector/include/libstatistics_collector/collector/collector.hpp
Show resolved
Hide resolved
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
2a54c8f
to
f16901d
Compare
I've addressed #117 (comment). Can you please re-review, @dabonnie, especially the changes to the |
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.
LGTM, but I can't approve my own PR.
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.
LGTM w/ ns cleaned-up
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.
LGTM.
You can skip the suggestions I left if you want.
I'd weigh the "move using
into unnamed namespace" suggestions higher than using auto.
libstatistics_collector/test/moving_average_statistics/test_moving_average_statistics.cpp
Outdated
Show resolved
Hide resolved
system_metrics_collector/test/system_metrics_collector/test_metrics_message_publisher.cpp
Show resolved
Hide resolved
...trics_collector/test/system_metrics_collector/test_linux_process_memory_measurement_node.cpp
Outdated
Show resolved
Hide resolved
..._metrics_collector/test/system_metrics_collector/test_linux_process_cpu_measurement_node.cpp
Show resolved
Hide resolved
system_metrics_collector/test/system_metrics_collector/test_linux_memory_measurement.cpp
Show resolved
Hide resolved
libstatistics_collector/test/topic_statistics_collector/test_received_message_period.cpp
Outdated
Show resolved
Hide resolved
libstatistics_collector/test/topic_statistics_collector/test_received_message_age.cpp
Outdated
Show resolved
Hide resolved
libstatistics_collector/test/moving_average_statistics/test_moving_average_statistics.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Miaofei <miaofei@amazon.com>
Create new libstatistics_collector library and copy relevant classes to new library
This is part of #224 to done.
Signed-off-by: Devin Bonnie dbbonnie@amazon.com