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

[exporter/signalfx] Fix memory leaks #31772

Merged
merged 18 commits into from Mar 25, 2024

Conversation

crobert-1
Copy link
Member

@crobert-1 crobert-1 commented Mar 14, 2024

Description:

Changes in PR:

  1. Add correlation client Shutdown function that blocks on the waitgroup. This is the main fix of this PR that should fix the leaking goroutines.
  2. Re-organize the shutdown process of the apm client correlation test suite to properly synchronize the shutting down process.
  3. Fix typo
  4. Only block request sender until context is cancelled. The request processor is shutdown when the context is cancelled, so this would result in Shutdown waiting forever, since the request would never be processed.
  5. Enable goleak in some more packages.

Note: This is contains the exact same contents as #30887, but change number 4 is new, and should resolve the test issue the original PR was causing.

Link to tracking Issue:
Resolves #30864
#30438

Testing:
All existing tests are passing, as well as added goleak checks. I'm going to run this a number of times to try to help ensure it's not flaky anymore.

@crobert-1 crobert-1 requested review from dmitryax and a team as code owners March 14, 2024 22:03
@crobert-1 crobert-1 marked this pull request as draft March 14, 2024 22:25
@crobert-1 crobert-1 marked this pull request as ready for review March 22, 2024 21:13
@crobert-1
Copy link
Member Author

This has passed 10+ times now without any failures. It should be ready to merge.

@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Mar 22, 2024
@mx-psi mx-psi merged commit 19eba68 into open-telemetry:main Mar 25, 2024
149 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 25, 2024
led0nk pushed a commit to led0nk/opentelemetry-collector-contrib that referenced this pull request Mar 25, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Changes in PR:

1. Add correlation client Shutdown function that blocks on the
waitgroup. This is the main fix of this PR that should fix the leaking
goroutines.
2. Re-organize the shutdown process of the apm client correlation test
suite to properly synchronize the shutting down process.
3. Fix typo
4. Only block request sender until context is cancelled. The request
processor is shutdown when the context is cancelled, so this would
result in `Shutdown` waiting forever, since the request would never be
processed.
5. Enable goleak in some more packages.

**Note**: This is contains the exact same contents as
open-telemetry#30887,
but change number 4 is new, and should resolve the test issue the
original PR was causing.

**Link to tracking Issue:**
Resolves
open-telemetry#30864

open-telemetry#30438

**Testing:** <Describe what testing was performed and which tests were
added.>
All existing tests are passing, as well as added goleak checks. I'm
going to run this a number of times to try to help ensure it's not flaky
anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/signalfx ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[exporter/signalfx] Failing goleak test
4 participants