Skip to content
This repository has been archived by the owner on Jun 10, 2021. It is now read-only.

Add statistic name and unit interfaces #115

Merged
merged 6 commits into from Mar 18, 2020

Conversation

prajakta-gokhale
Copy link
Member

@prajakta-gokhale prajakta-gokhale commented Mar 15, 2020

Changes to topic_statistics_collector interface. Needed for #112.

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

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

@codecov
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #115 into master will increase coverage by 0.07%.
The diff coverage is 48.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   38.70%   38.77%   +0.07%     
==========================================
  Files          38       39       +1     
  Lines        1465     1488      +23     
  Branches      892      908      +16     
==========================================
+ Hits          567      577      +10     
+ Misses         72       67       -5     
- Partials      826      844      +18     
Flag Coverage Δ
#unittests 38.77% <48.78%> (+0.07%) ⬆️
Impacted Files Coverage Δ
...llector/src/system_metrics_collector/collector.hpp 100.00% <ø> (ø)
...m_metrics_collector/linux_cpu_measurement_node.hpp 100.00% <ø> (ø)
...etrics_collector/linux_memory_measurement_node.hpp 100.00% <ø> (ø)
...s_collector/linux_process_cpu_measurement_node.hpp 100.00% <ø> (ø)
...ollector/linux_process_memory_measurement_node.hpp 100.00% <ø> (ø)
...em_metrics_collector/metrics_message_publisher.hpp 100.00% <ø> (ø)
...tatistics_collector/topic_statistics_collector.hpp 100.00% <ø> (ø)
...tistics_collector/test_received_message_period.cpp 18.75% <25.00%> (+0.89%) ⬆️
...r/test/system_metrics_collector/test_collector.cpp 46.15% <42.85%> (-0.52%) ⬇️
...trics_collector/test_periodic_measurement_node.cpp 27.38% <42.85%> (+1.58%) ⬆️
... and 11 more

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 6e9d44c...a4ec797. Read the comment docs.

Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thomas-moulard
Copy link
Member

The coverage seems to be decreasing with this PR, is there any easy fix for us to try not to reduce coverage on this?

Prajakta Gokhale added 4 commits March 17, 2020 13:50
Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
@prajakta-gokhale
Copy link
Member Author

The coverage seems to be decreasing with this PR, is there any easy fix for us to try not to reduce coverage on this?

After rebasing on origin/master it seems to be decreasing by 0.17%. The only new code added is an interface whose implementation is unit tested in concrete derived classes.

Increasing it would be a non-trivial change.

@thomas-moulard
Copy link
Member

No worries, this is fine. On the report:

https://codecov.io/gh/ros-tooling/system_metrics_collector/pull/115/diff?src=pr&el=tree-more

test_collector.cpp GetMetricName/Unit methods are not hit currently, just FYI.

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

@dabonnie dabonnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@prajakta-gokhale prajakta-gokhale merged commit e672ed0 into master Mar 18, 2020
@prajakta-gokhale prajakta-gokhale deleted the prajaktg/tsc-interface branch March 18, 2020 01:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants