department-of-veterans-affairs / abd-vro

To get Veterans benefits in minutes, VRO software uses health evidence data to help fast track disability claims.
Other
19 stars 6 forks source link

EP Merge: Fix memory leak #3097

Closed dfitchett closed 3 months ago

dfitchett commented 3 months ago

What was the problem?

While working on #2783 to make the calls to datadog asynchronous, I noticed that the current implementation of EP Merge app uses creates an asyncio event loop but never closes this loop. The investigation/troubleshooting on that ticket, led me to realize we had a memory leak as was evidenced by the dashboard chart linked in the description.

I proceeded to run some tests locally to verify that was in fact the issue.

The 5 minute fixed rate tests demonstrated this effect. The tests sent 2 concurrent requests / second to the EP merge app deployed to a docker container. This allows independent tracking of memory and CPU usage similar to that of a container deployed on LHDI. Note, the traffic, however, is not similar because in production we hardly ever receive concurrent requests due to such a narrowed scope in eligible merges.

Tests

Cases:

  1. Currently deployed functionality of not closing the asyncio event loop
  2. Closing created event loop using asyncio.run(...)

Setup:

Results

Case 1: current deployment

Image

Case 2: closing loop

Image

How does this fix it?[^1]

Uses asyncio.run(...) which closes the event loop upon finishing coroutine.

How to test this PR

github-actions[bot] commented 3 months ago

Test Results

116 tests  ±0   116 :white_check_mark: ±0   49s :stopwatch: +9s  34 suites ±0     0 :zzz: ±0   34 files   ±0     0 :x: ±0 

Results for commit 1fa6e9dd. ± Comparison against base commit dffaadb2.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 3 months ago

JaCoCo Test Coverage

Overall Project 68.83% :x:

There is no coverage information present for the Files changed