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

Check goroutines in places related to recent found leaks #12106

Merged
merged 20 commits into from
May 24, 2019

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented May 8, 2019

Follows the strategy used in #10850 to check for goroutine leaks
on tests in some places related to the leaks investigated as part
of #9302.

Several things here:

@jsoriano jsoriano added bug Filebeat Filebeat libbeat :Testing Team:Integrations Label for the Integrations team labels May 8, 2019
@jsoriano jsoriano requested a review from a team as a code owner May 8, 2019 19:37
@jsoriano jsoriano self-assigned this May 8, 2019
filebeat/input/log/input.go Outdated Show resolved Hide resolved
@jsoriano jsoriano requested a review from exekias May 8, 2019 19:47
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Do our runtime metrics that get logged every 30s include a gauge for the number of goroutines? That would be helpful for identifying if a leak is introduced to a Beat.

libbeat/tests/resources/goroutines.go Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member Author

jsoriano commented May 9, 2019

I am moving the fix for the leak in NewInput to its own PR (#12125), so we can continue discussing about these tests here without blocking this fix.

@jsoriano
Copy link
Member Author

jsoriano commented May 9, 2019

Do our runtime metrics that get logged every 30s include a gauge for the number of goroutines? That would be helpful for identifying if a leak is introduced to a Beat.

Created PR for that #12135

filebeat/input/log/input.go Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member Author

Failing tests caused by #12171

@jsoriano
Copy link
Member Author

@exekias @andrewkroh this would be ready for review, thanks!

@jsoriano
Copy link
Member Author

Test failure in Windows looks related, I'll look at this

time.Sleep(10 * time.Millisecond)
}
profile := pprof.Lookup("goroutine")
profile.WriteTo(os.Stdout, 2)
Copy link
Member

Choose a reason for hiding this comment

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

Nice. That's helpful.

@jsoriano jsoriano merged commit f841521 into elastic:master May 24, 2019
@jsoriano jsoriano deleted the test-close-on-signal branch May 24, 2019 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants