awslabs / aws-embedded-metrics-dotnet

Amazon CloudWatch Embedded Metric Format Client Library
Apache License 2.0
22 stars 14 forks source link

Fix canary's heap usage to avoid system crash #46

Closed meshwa19 closed 1 year ago

meshwa19 commented 1 year ago

Issue #40

Description of changes:

When we create a new instance of MetricsLogger in the Canary, the program is not able to detect the existing connection and makes a new connection every time. Hence, it reconnects after every packet it sends and ends up creating multiple open ports. The reason behind this is the variable _cachedEnvironment is not static, and hence, it is not shared across all instances of the class. After making this variable static, when a new logger is created, it will be able to find the existing connection (if any) and won't make a new connection.

Also changed docker image for fixing the build pipeline failure. And finally, as tests run in parallel for dotnet, making the environment static was leading to some test failures. Hence, cleaned the environment after every tests.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

markkuhn commented 1 year ago

Why aren't we making _tcpClient static here?

meshwa19 commented 1 year ago

Build is failing. Please take a look.

Fixed the build (a32e2e1). Not the best solution as there is code redundancy. Looking into improving it.

meshwa19 commented 1 year ago

Why aren't we making _tcpClient static here?

Here, '_tcpClient' is an object of class System.Net.Sockets.TcpClient and later gets instantiated where it connects to the specified port on the specified host. As this '_tcpClient' variable is an object of an inbuilt dotnet library, I believe it won't be a good practice to make it static.

Also, as '_cachedEnvironment' is not retained, a new instance of TCPClient class (the wrapper class we have built which internally uses System.Net.Sockets.TcpClient class) is created everytime which is not ideal. Hence, I made the '_cached environment' variable as static. Lastly, I have taken reference from the EMF Java Library which also does the same thing.

gordonpn commented 1 year ago

I didn't see any testing notes, how was this tested?

Code changes looks good to me.

meshwa19 commented 1 year ago

I didn't see any testing notes, how was this tested?

Code changes looks good to me.

So, I monitored the Canary's memory usage on CWL Dashboard. See the snapshots of before and after the change:

Before:

Screen Shot 2022-12-09 at 8 46 00 AM

After:

Screen Shot 2022-12-09 at 8 45 37 AM

Also, see the latest build for Canary on my PR.

Screen Shot 2022-12-09 at 8 43 10 AM