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

Initial support for GELF HTTP transport #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Initial support for GELF HTTP transport #37

wants to merge 3 commits into from

Conversation

joschi
Copy link
Contributor

@joschi joschi commented Feb 5, 2018

Closes #9

@joschi joschi requested a review from bernd February 5, 2018 15:12
@coveralls
Copy link

coveralls commented Feb 5, 2018

Coverage Status

Coverage increased (+4.8%) to 84.135% when pulling 56bf286 on gelf-http into c795a5a on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.5%) to 87.127% when pulling dcd71d0 on gelf-http into a55396e on master.

Copy link
Member

@bernd bernd left a comment

Choose a reason for hiding this comment

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

There are some issues with this PR that we need to fix.

.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, config.getConnectTimeout())
.option(ChannelOption.TCP_NODELAY, config.isTcpNoDelay())
.option(ChannelOption.SO_KEEPALIVE, config.isTcpKeepAlive())
.remoteAddress(config.getRemoteAddress())
Copy link
Member

Choose a reason for hiding this comment

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

This is not taking the GelfConfiguration#getUri() configuration option into account. (see comment in GelfConfiguration.java)

}

public GelfConfiguration uri(URI uri) {
this.uri = uri;
Copy link
Member

Choose a reason for hiding this comment

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

The uri is set but it's not used in #getRemoteAddress(). We should probably check if the uri field is set and use it as an override for the hostname and port fields.

Copy link
Member

Choose a reason for hiding this comment

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

The getUri() and uri() methods are also missing javadoc.


@Override
public void channelInactive(ChannelHandlerContext ctx) throws Exception {
LOG.info("Channel disconnected!");
Copy link
Member

Choose a reason for hiding this comment

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

I see lots of these errors when running against a real HTTP server and it also drops messages.

[gelfHttpTransport-1-1] INFO org.graylog2.gelfclient.transport.GelfHttpTransport - Channel disconnected!
[gelfHttpTransport-1-1] ERROR org.graylog2.gelfclient.encoder.GelfHttpEncoder - Error while encoding HTTP request
java.io.IOException: Connection reset by peer
	at sun.nio.ch.FileDispatcherImpl.read0(Native Method)
	at sun.nio.ch.SocketDispatcher.read(SocketDispatcher.java:39)
	at sun.nio.ch.IOUtil.readIntoNativeBuffer(IOUtil.java:223)
	at sun.nio.ch.IOUtil.read(IOUtil.java:192)
	at sun.nio.ch.SocketChannelImpl.read(SocketChannelImpl.java:380)
	at io.netty.buffer.PooledByteBuf.setBytes(PooledByteBuf.java:247)
	at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:1147)
	at io.netty.channel.socket.nio.NioSocketChannel.doReadBytes(NioSocketChannel.java:347)
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:148)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:700)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:635)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:552)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:514)
	at io.netty.util.concurrent.SingleThreadEventExecutor$6.run(SingleThreadEventExecutor.java:1044)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.lang.Thread.run(Thread.java:748)
[gelfHttpTransport-1-1] INFO org.graylog2.gelfclient.transport.GelfHttpTransport - Channel disconnected!
[gelfHttpTransport-1-1] INFO org.graylog2.gelfclient.transport.GelfHttpTransport - Channel disconnected!
[gelfHttpTransport-1-1] ERROR org.graylog2.gelfclient.encoder.GelfHttpEncoder - Error while encoding HTTP request
java.io.IOException: Connection reset by peer
	at sun.nio.ch.FileDispatcherImpl.read0(Native Method)
	at sun.nio.ch.SocketDispatcher.read(SocketDispatcher.java:39)
	at sun.nio.ch.IOUtil.readIntoNativeBuffer(IOUtil.java:223)
	at sun.nio.ch.IOUtil.read(IOUtil.java:192)
	at sun.nio.ch.SocketChannelImpl.read(SocketChannelImpl.java:380)
	at io.netty.buffer.PooledByteBuf.setBytes(PooledByteBuf.java:247)
	at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:1147)
	at io.netty.channel.socket.nio.NioSocketChannel.doReadBytes(NioSocketChannel.java:347)
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:148)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:700)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:635)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:552)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:514)
	at io.netty.util.concurrent.SingleThreadEventExecutor$6.run(SingleThreadEventExecutor.java:1044)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.lang.Thread.run(Thread.java:748)
[gelfHttpTransport-1-1] INFO org.graylog2.gelfclient.transport.GelfHttpTransport - Channel disconnected!
[gelfHttpTransport-1-1] INFO org.graylog2.gelfclient.transport.GelfHttpTransport - Channel disconnected!

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ bernd
❌ Jochen Schalanda


Jochen Schalanda seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add HTTP transport
4 participants