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

Add Logentries handler. #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Logentries handler. #33

wants to merge 1 commit into from

Conversation

hwrdprkns
Copy link

This handler will report to LogEntries with a TCP token.

@hwrdprkns
Copy link
Author

image

@hwrdprkns
Copy link
Author

@tj @phacops - ping

@jchv
Copy link

jchv commented Jan 25, 2017

Hey, this would be useful to me. Is there any chance of this being upstreamed soon?

cc: @tj

@tj
Copy link
Member

tj commented Jan 25, 2017

hmm weird I thought I commented on this one, added

@tj
Copy link
Member

tj commented Jan 25, 2017

LGTM other than the potentially superfluous Close

@hwrdprkns
Copy link
Author

@tj @johnwchadwick - Just rebased - a review would be great. I imagine in long running situations the HandleLog function could return an error if the socket gets closed or borked. May have to call NewLogHandler in that case.

@tj
Copy link
Member

tj commented Jan 25, 2017

@johnwchadwick care to check it out? I don't use logentries

@jchv
Copy link

jchv commented Jan 26, 2017

Oh, I think the current code is probably sufficient. The le_go library does ensure the TCP socket is open and will attempt to reconnect if not, and if that fails it will return an error.

That being said, I definitely think the Close is superfluous. In fact, if it weren't for the fact that le_go ensures the socket is open, that defer Close would probably cause a problem since it runs before the handler is even returned from the constructor.

@tj
Copy link
Member

tj commented Jan 26, 2017

yep, cool. I'll tweak a few things and get that in tomorrow. We're missing comments and the style is a bit different than the others (they all use New() for ex). OCD but hey haha, I like keeping things consistent.

This handler will report to LogEntries with a [TCP token](https://github.com/bsphere/le_go#usage).
return nil, err
}

return &LogHandler{
Copy link
Author

Choose a reason for hiding this comment

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

Wasn't 100% sure here if I could nix the previous guard and return &LogHandler{logger: logger}, err here

Copy link

@jchv jchv Jan 26, 2017

Choose a reason for hiding this comment

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

I think it's best to return nil on error. (in other words, I think the guard is good.)

@hwrdprkns
Copy link
Author

hwrdprkns commented Jan 26, 2017

Please add comments to your content!

@jchv
Copy link

jchv commented Jan 27, 2017

Hmm. I'm running some tests with this in a close-to-production environment and I actually see some issues with the reliability. It seemed to arbitrarily disconnect silently and not reconnect. It's kind of strange given that bsphere/le_go seems to have it's own reconnection logic.

Well, YMMV. I may try to roll a version that doesn't have external dependencies for my own usage, since the LE protocol is pretty braindead-simple anyways.

@jchv
Copy link

jchv commented Jan 27, 2017

And, now I've done that. My LogEntries handler is available here. Of course, there's no guarantee that my implementation of the LogEntries protocol is better than bsphere/le_go. I did it because I am experiencing issues in my environment and because I wanted more control. That being said, if there's ever any interest in upstreaming this version, let me know.

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.

3 participants