opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[service] Fix memory leaks and enable goleak check in tests

Open crobert-1 opened this issue 1 year ago • 8 comments

Description: <Describe what has changed.>

This change adds goleak to check for memory leaks. Originally there were 3 failing tests in the service package, so I'll describe changes in relation to resolving each test's failing goleak check.

  1. TestServiceTelemetryRestart: Simplest fix, close the response body to make sure goroutines aren't leaked by reopening a server on the same port. This was just a test issue
  2. TestTelemetryInit.UseOTelWithSDKConfiguration: The meter provider was being started in the initialization process (metrics reference), but never shutdown. The type originally being used (meter.MetricProvider) was the base interface which didn't provide a Shutdown method. I changed this to use the sdk interfaces that provide the required Shutdown method. The actual functionality of starting the providers was already using and returning the sdk interface, so the actual underlying type remains the same. Since mp is a private member and sdkmetric and implement the original type, I don't believe changing the type is a breaking change.
  3. TestServiceTelemetryCleanupOnError: This test starts a server using a sub-goroutine, cancels it, starts again in a subroutine, and cancels again in the main goroutine. This test showed the racing behavior of the subroutine running server.ListenAndServe and the main goroutine's functionality of calling close and then starting the server again right away. The solution here is to add a sync.WaitGroup variable that can properly block until all servers are closed before returning from shutdown. This will allow us to ensure it's safe to proceed knowing the ports are free, and server is fully closed.

The first test fix was just a test issue, but 2 and 3 were real bugs. I realize it's a bit hard to read with them all together, but I assumed adding PR dependency notes would be more complicated.

Link to tracking Issue: <Issue number if applicable> #9165

Testing: <Describe what testing was performed and which tests were added.> All tests are passing as well as goleak check.

crobert-1 avatar Jan 08 '24 21:01 crobert-1

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.63%. Comparing base (ef07ea0) to head (a81993e).

Files Patch % Lines
service/internal/proctelemetry/config.go 75.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9241      +/-   ##
==========================================
- Coverage   91.64%   91.63%   -0.01%     
==========================================
  Files         406      406              
  Lines       19001    19007       +6     
==========================================
+ Hits        17414    17418       +4     
- Misses       1227     1229       +2     
  Partials      360      360              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 08 '24 21:01 codecov[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Feb 03 '24 03:02 github-actions[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Feb 27 '24 03:02 github-actions[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Mar 22 '24 03:03 github-actions[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar May 02 '24 03:05 github-actions[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar May 22 '24 03:05 github-actions[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Jun 09 '24 03:06 github-actions[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Jun 25 '24 03:06 github-actions[bot]

@crobert-1 What's the status of this PR? Does it need more reviews? Do you need to fix something? (No rush, just trying to clean up our backlog)

mx-psi avatar Jul 18 '24 11:07 mx-psi

@crobert-1 What's the status of this PR? Does it need more reviews? Do you need to fix something? (No rush, just trying to clean up our backlog)

Just waiting for reviews 👍

crobert-1 avatar Jul 18 '24 15:07 crobert-1

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Aug 09 '24 03:08 github-actions[bot]

Can you fix the merge conflicts? I can merge after that

Should be good now, thanks @mx-psi!

crobert-1 avatar Aug 12 '24 16:08 crobert-1