eProsima / Fast-DDS-statistics-backend

eProsima Fast DDS Statistics Backend is a C++ library that provides collection and procession the statistics measurements reported by Fast DDS Statistics Module. Looking for commercial support? Contact info@eprosima.com
https://eprosima.com
Apache License 2.0
17 stars 9 forks source link

Memory usage performance test [15785] #170

Open jparisu opened 1 year ago

jparisu commented 1 year ago

Merge after:

codecov[bot] commented 1 year ago

Codecov Report

Base: 58.42% // Head: 58.42% // No change to project coverage :thumbsup:

Coverage data is based on head (d7bd7bf) compared to base (74f5197). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #170 +/- ## ======================================= Coverage 58.42% 58.42% ======================================= Files 33 33 Lines 4421 4421 Branches 2352 2352 ======================================= Hits 2583 2583 Misses 54 54 Partials 1784 1784 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eProsima). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eProsima)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

MiguelCompany commented 1 year ago

@EduPonz I think I don't have enough knowledge about bash scripting to review this one ...

MiguelCompany commented 1 year ago

@jparisu Please rebase this one

EduPonz commented 1 year ago

I have rebased the PR while looking at it. I think we ought to have a discussion about this; right now, it's a draft as it is far from being ready to be incorporated in CI and merged. The pain points I see with the current for merging as a test are:

Another possibility would be to add the script as a resource from which users can benefit, but I'm really attracted to the idea of having this kind of stress tests.

jparisu commented 1 year ago

Another possibility would be to add the script as a resource from which users can benefit, but I'm really attracted to the idea of having this kind of stress tests.

This test was never meant to be added in the CI. This was intended to be a manual test to have a partial idea of what is the memory usage of an execution. In order to automatize it and have a failing criteria I think it could not be done as results highly depend on the architecture, OS and state of the machine where tests are running. I think it would be better to have it as a helper script rather than an actual test (does Fast DDS have memory usage automatized tests with failing criteria?).

PD: I personally think we have bigger problems that having a stress test.

jparisu commented 1 year ago

This is an image of the results obtained by running this test: image

EduPonz commented 1 year ago

I completely agree. I'd leave this as a draft for now and we'll see in the future

JLBuenoLopez commented 1 year ago

@jparisu which is the status of this Draft? Does it make sense to try to merge it? I think we should change the status and assign a milestone so the discussion is had at some point.