Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add memory leak detection to unit tests #3405

Open
dblock opened this issue Feb 9, 2023 · 3 comments
Open

Add memory leak detection to unit tests #3405

dblock opened this issue Feb 9, 2023 · 3 comments
Labels

Comments

@dblock
Copy link
Member

dblock commented Feb 9, 2023

Is your feature request related to a problem? Please describe.

In #1427 a memory leak was introduced and fixed in #3390. Is there some static analysis that could have caught it in the PR?

Describe the solution you'd like

Integrate memory leak detection into unit/integration tests. For example, https://github.com/andywer/leakage.

@joshuarrrr
Copy link
Member

Another potential tool: https://engineering.fb.com/2022/09/12/open-source/memlab/

@ashwin-pc
Copy link
Member

@joshuarrrr Thats a really neat tool. I was thinking of a slightly different approach here. We already have a way to validate this on opensearch afaik. The way they do it is by exposing an api that reports the memory stats when it is queried. They then run a synthetic load against a test cluster and capture the stats and then analyse it to identify any memory leaks. We too have a similar api the /status endpoint. It has a pretty neat UI too. We can run a similar synthetic workload on dashboards using the same tool.

Screenshot 2023-04-04 at 6 06 37 AM

@dblock Just like OpenSearch, i think running such a workload for each PR can be quite expensive as either a unit or integ test. I think its best if we run it during builds since they are less frequent and can be more through with the synthetic workloads.

@dblock
Copy link
Member Author

dblock commented Apr 21, 2023

I think any technology and any approach that can confidently say that #3405 would have been found before release, would work for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants