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

Fix splunksql DB goroutine leak #1682

Merged
merged 6 commits into from
Jan 31, 2023
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jan 27, 2023

fix #1676
fix #1677

The Open function from the splunksql instrumentation opens a database to determine the underlying connector or driver. It never closes this database. Update the function that returns the initial database in a way where a user closes the returned database also closes the initial database.

This resolves the issue of the database goroutine being abandoned.

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023

Codecov Report

Base: 81.86% // Head: 81.93% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (77c5077) compared to base (a4073e9).
Patch coverage: 89.28% of modified lines in pull request are covered.

❗ Current head 77c5077 differs from pull request most recent head 5056744. Consider uploading reports for the commit 5056744 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1682      +/-   ##
==========================================
+ Coverage   81.86%   81.93%   +0.06%     
==========================================
  Files          63       63              
  Lines        2625     2651      +26     
==========================================
+ Hits         2149     2172      +23     
- Misses        428      430       +2     
- Partials       48       49       +1     
Flag Coverage Δ
Linux 81.55% <89.28%> (+0.06%) ⬆️
Windows 78.14% <89.28%> (+0.11%) ⬆️
macOS ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
instrumentation/database/sql/splunksql/sql.go 86.66% <86.95%> (-0.52%) ⬇️
...nstrumentation/database/sql/splunksql/connector.go 80.00% <100.00%> (+10.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

If the underlying types implement Close as well, the call to Close will
be forwarded to them.
@MrAlias MrAlias marked this pull request as ready for review January 30, 2023 20:58
@MrAlias MrAlias requested review from a team as code owners January 30, 2023 20:58
Copy link
Contributor

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

PS. I spent about ~2 hours reviewing this PR... And I was adding and later deleting comments a few times 😆 But everything is well structured, tested, and the comments are great!

instrumentation/database/sql/splunksql/sql.go Show resolved Hide resolved
@MrAlias MrAlias requested a review from a team as a code owner January 31, 2023 17:45
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@MrAlias MrAlias enabled auto-merge (squash) January 31, 2023 17:56
@MrAlias MrAlias merged commit 824d1c1 into signalfx:main Jan 31, 2023
@MrAlias MrAlias deleted the splunksql-fix-leak branch January 31, 2023 18:13
@MrAlias MrAlias mentioned this pull request Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

splunksql: Not closing the underlying driver.Connector splunksql: Goroutine leak
3 participants