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

Create graygelf in constructor once vs on every call to eventHandler #222

Merged
merged 2 commits into from
Apr 16, 2020

Conversation

hkclark
Copy link
Contributor

@hkclark hkclark commented Apr 15, 2020

Here is a Pull Request that fixes the leak with graygelf for me. Let me know if there are any questions or comments or if I can do anything to help. Thanks!

@otisg
Copy link
Member

otisg commented Apr 16, 2020

Ping @adnanrahic, @megastef can we merge & release?

@adnanrahic adnanrahic self-requested a review April 16, 2020 07:26
Copy link
Contributor

@adnanrahic adnanrahic left a comment

Choose a reason for hiding this comment

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

@hkclark Hey! This looks good to me. But, there is a merge conflict due to your previous PR #220 being merged. If you merge origin/master into your hkclark:fix_#221_graygelf_leak branch, and push it once again, the conflict should be resolved.

@hkclark
Copy link
Contributor Author

hkclark commented Apr 16, 2020

@adnanrahic Yeah, I almost did this assuming the other was pushed but figured it was better to do them both right off master. Will do as you suggest... should be sorted in a sec.

@hkclark hkclark force-pushed the fix_#221_graygelf_leak branch 2 times, most recently from 78fb0ef to a7a11c4 Compare April 16, 2020 11:35
@hkclark
Copy link
Contributor Author

hkclark commented Apr 16, 2020

@adnanrahic Whew, I should never do this stuff before morning coffee! But I should have it sorted. Let me know if you see any issues. Thanks!

@adnanrahic adnanrahic self-requested a review April 16, 2020 12:23
Copy link
Contributor

@adnanrahic adnanrahic 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! Oh yeah, I understand all too well. I'll go ahead and merge this right away, and release a new version today. 👌

@adnanrahic
Copy link
Contributor

Thanks again @hkclark for taking the time to fix this issue. Hope you enjoy using Logagent. Feel free to reach out or open new issues if you come across any bugs. 😄

@adnanrahic adnanrahic merged commit c6173db into sematext:master Apr 16, 2020
@adnanrahic adnanrahic linked an issue Apr 16, 2020 that may be closed by this pull request
@hkclark
Copy link
Contributor Author

hkclark commented Apr 16, 2020

@adnanrahic Terrific! Thank you so much for the help. We are looking forward to using logagent.js -- it's got some really great features and a nice, lightweight footprint. Many thanks!

@otisg
Copy link
Member

otisg commented Apr 16, 2020

Thank you for your kind words, @hkclark!
According to https://github.com/hkclark this was your first PR on GitHub? If that's right, 👏.

@hkclark
Copy link
Contributor Author

hkclark commented Apr 16, 2020

Yeah, we use a private Github Enterprise and a bunch of other private git repos at work, so I haven't done much on the public side of things. :-)

@otisg
Copy link
Member

otisg commented Apr 16, 2020

Ah, and I thought we were your first love ;)

@hkclark
Copy link
Contributor Author

hkclark commented Apr 16, 2020

LOL :-)

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.

output-gelf plugin uses up all ports and crashes
3 participants