Add SubscriberTopicStatisticsNode and tests #112

prajakta-gokhale · 2020-03-12T22:05:01Z

Related to https://github.com/ros-tooling/aws-roadmap/issues/197

Signed-off-by: Prajakta Gokhale prajaktg@amazon.com

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp

mm318

A lot of the code in SubscriberTopicStatistics look the same as the PeriodicMeasurementNode. Can they be refactored to have a common base class?

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

codecov · 2020-03-20T09:06:37Z

Codecov Report

Merging #112 into master will increase coverage by 1.52%.
The diff coverage is 41.63%.

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   33.54%   35.06%   +1.52%     
==========================================
  Files          26       29       +3     
  Lines        1106     1363     +257     
  Branches      724      878     +154     
==========================================
+ Hits          371      478     +107     
- Misses         63       81      +18     
- Partials      672      804     +132

Flag	Coverage Δ
#unittests	`35.06% <41.63%> (+1.52%)`	⬆️

Impacted Files	Coverage Δ
...ics_collector/test_subscriber_topic_statistics.cpp	`35.84% <35.84%> (ø)`
...atistics_collector/subscriber_topic_statistics.hpp	`44.87% <44.87%> (ø)`
...src/topic_statistics_collector/parameter_utils.hpp	`75.00% <75.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22f8de5...0564745. Read the comment docs.

system_metrics_collector/src/topic_statistics_collector/constants.hpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp

dabonnie

This is a fairly large PR (currently +849) and isn't the easiest to review: I probably need to make another pass. If possible I would suggest breaking this up.

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

prajakta-gokhale · 2020-03-25T02:34:49Z

This is a fairly large PR (currently +849) and isn't the easiest to review: I probably need to make another pass. If possible I would suggest breaking this up.

Refactored version is smaller and more straight-forward imo. Figured it's easier to view class+unit tests together, if not I can separate the two into different PRs.

dabonnie

This is getting close. Once the lifecycle transitions are fixed this is pretty much done.

system_metrics_collector/src/topic_statistics_collector/constants.hpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

emersonknapp

minor non-blocking nits

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

thomas-moulard · 2020-03-31T17:53:36Z

system_metrics_collector/package.xml

@@ -27,6 +27,7 @@
  <test_depend>ament_lint_common</test_depend>
  <test_depend>class_loader</test_depend>
  <test_depend>lifecycle_msgs</test_depend>
+  <test_depend>sensor_msgs</test_depend>


can we avoid this by using a dummy message in the test folder?

Yes it is possible to use a dummy message with Header. Currently multiple packages use IMU messages in tests, I have filed #120 to change them.

thomas-moulard · 2020-03-31T18:07:01Z

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp

+/**
+ * Class which makes periodic topic statistics measurements, using a ROS2 timer.
+ */
+template<typename T>


Please document what is T here. Can we add a static_assert to make sure only "allowed types" are passed?

T here is for any ROS 2 message type. I'm not sure how we can enforce that using static_assert?

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch 2 times, most recently from 988bb36 to da58418 Compare March 12, 2020 23:08

dabonnie suggested changes Mar 13, 2020

View reviewed changes

piraka9011 reviewed Mar 13, 2020

View reviewed changes

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp Outdated Show resolved Hide resolved

piraka9011 reviewed Mar 13, 2020

View reviewed changes

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp Outdated Show resolved Hide resolved

piraka9011 reviewed Mar 13, 2020

View reviewed changes

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp Outdated Show resolved Hide resolved

zmichaels11 reviewed Mar 13, 2020

View reviewed changes

mm318 reviewed Mar 13, 2020

View reviewed changes

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp Outdated Show resolved Hide resolved

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.cpp Outdated Show resolved Hide resolved

thomas-moulard reviewed Mar 13, 2020

View reviewed changes

prajakta-gokhale mentioned this pull request Mar 15, 2020

Add statistic name and unit interfaces #115

Merged

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from da58418 to 67f0fc5 Compare March 20, 2020 08:42

prajakta-gokhale marked this pull request as ready for review March 20, 2020 08:43

prajakta-gokhale changed the title ~~[WIP] Add subscriber_topic_statistics class and tests~~ Add SubscriberTopicStatisticsNode and tests Mar 20, 2020

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from 67f0fc5 to e57a4d1 Compare March 20, 2020 09:40

prajakta-gokhale changed the title ~~Add SubscriberTopicStatisticsNode and tests~~ [WIP] Add SubscriberTopicStatisticsNode and tests Mar 20, 2020

prajakta-gokhale changed the title ~~[WIP] Add SubscriberTopicStatisticsNode and tests~~ Add SubscriberTopicStatisticsNode and tests Mar 20, 2020

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch 3 times, most recently from 2b8b9ba to 63a0924 Compare March 20, 2020 23:07

prajakta-gokhale requested review from dabonnie and emersonknapp March 20, 2020 23:08

emersonknapp suggested changes Mar 23, 2020

View reviewed changes

zmichaels11 reviewed Mar 23, 2020

View reviewed changes

dabonnie suggested changes Mar 24, 2020

View reviewed changes

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from 98825b6 to 5202ef8 Compare March 25, 2020 01:20

prajakta-gokhale requested a review from dabonnie March 26, 2020 00:52

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from 5202ef8 to 176b502 Compare March 26, 2020 01:59

prajakta-gokhale requested a review from emersonknapp March 26, 2020 02:14

dabonnie suggested changes Mar 26, 2020

View reviewed changes

zmichaels11 reviewed Mar 26, 2020

View reviewed changes

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp Outdated Show resolved Hide resolved

system_metrics_collector/test/topic_statistics_collector/test_subscriber_topic_statistics.cpp Outdated Show resolved Hide resolved

prajakta-gokhale mentioned this pull request Mar 26, 2020

Allow topic statistics to subscribe to multiple topics #118

Closed

3 tasks

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from 176b502 to 3939d08 Compare March 26, 2020 18:51

zmichaels11 suggested changes Mar 27, 2020

View reviewed changes

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp Outdated Show resolved Hide resolved

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp Outdated Show resolved Hide resolved

prajakta-gokhale requested a review from dabonnie March 30, 2020 23:42

Prajakta Gokhale added 5 commits March 30, 2020 21:29

Add subscriber_topic_statistics class and tests

6eb9a20

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

Rebase on changes in interfaces, address feedback

032f148

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

Fix unit test and try to increase coverage

dbea19b

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

Address review feedback

d9ea035

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

Fix incorrect handling of publish timer and review comments

65972ce

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from f4831cb to 905edff Compare March 31, 2020 05:32

dabonnie approved these changes Mar 31, 2020

View reviewed changes

Resolve conflicts

c197e22

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from 905edff to c197e22 Compare March 31, 2020 17:32

prajakta-gokhale requested a review from zmichaels11 March 31, 2020 17:35

zmichaels11 approved these changes Mar 31, 2020

View reviewed changes

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp Show resolved Hide resolved

system_metrics_collector/src/topic_statistics_collector/subscriber_topic_statistics.hpp Show resolved Hide resolved

emersonknapp approved these changes Mar 31, 2020

View reviewed changes

mm318 reviewed Mar 31, 2020

View reviewed changes

thomas-moulard reviewed Mar 31, 2020

View reviewed changes

prajakta-gokhale mentioned this pull request Mar 31, 2020

Use a dummy message instead of sensor_msgs in tests and demos #120

Closed

New utility header and more review comments

0564745

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>

prajakta-gokhale force-pushed the prajaktg/subscriber-topic-stats branch from e18c89f to 0564745 Compare March 31, 2020 23:36

prajakta-gokhale merged commit aae3cb5 into master Apr 1, 2020

prajakta-gokhale deleted the prajaktg/subscriber-topic-stats branch April 1, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SubscriberTopicStatisticsNode and tests #112

Add SubscriberTopicStatisticsNode and tests #112

prajakta-gokhale commented Mar 12, 2020 •

edited

mm318 left a comment

codecov bot commented Mar 20, 2020 •

edited

dabonnie left a comment

prajakta-gokhale commented Mar 25, 2020

dabonnie left a comment

emersonknapp left a comment

thomas-moulard Mar 31, 2020

prajakta-gokhale Mar 31, 2020

thomas-moulard Mar 31, 2020

prajakta-gokhale Mar 31, 2020

Add SubscriberTopicStatisticsNode and tests #112

Add SubscriberTopicStatisticsNode and tests #112

Conversation

prajakta-gokhale commented Mar 12, 2020 • edited

mm318 left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 20, 2020 • edited

Codecov Report

dabonnie left a comment

Choose a reason for hiding this comment

prajakta-gokhale commented Mar 25, 2020

dabonnie left a comment

Choose a reason for hiding this comment

emersonknapp left a comment

Choose a reason for hiding this comment

thomas-moulard Mar 31, 2020

Choose a reason for hiding this comment

prajakta-gokhale Mar 31, 2020

Choose a reason for hiding this comment

thomas-moulard Mar 31, 2020

Choose a reason for hiding this comment

prajakta-gokhale Mar 31, 2020

Choose a reason for hiding this comment

prajakta-gokhale commented Mar 12, 2020 •

edited

codecov bot commented Mar 20, 2020 •

edited