Added GelfHTTPSender #68
Conversation
…URI if not present.
|
Thanks for your PR. I'll take a deeper look on it. Could you rebase the PR as there are merge conflicts? |
| @Before | ||
| public void prepareMocks() throws IOException { | ||
| MockitoAnnotations.initMocks(this); | ||
| // when(closeableHttpResponse.getStatusLine()).thenReturn(new BasicStatusLine(new ProtocolVersion("HTTP", 1, 1), 202, "Accepted")); |
There was a problem hiding this comment.
An integration test with the sender would be great. Netty is already on the class-path, you can build your own HTTP server, see https://github.com/netty/netty/tree/4.0/example/src/main/java/io/netty/example/http/helloworld
There was a problem hiding this comment.
FWIW, using https://github.com/square/okhttp/tree/master/mockwebserver#readme might be useful for those kind of tests without writing everything from scratch. IMHO OkHttp is also quite a good HTTP client library.
There was a problem hiding this comment.
Thx. I'm a bit concerned about pulling in dependencies. With one "lousy" logger appender you're forced to a dependency that might kill your app :(
There was a problem hiding this comment.
This might be a good case for optional dependencies (https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html) or using the HTTP client included in the JDK (https://docs.oracle.com/javase/8/docs/api/java/net/HttpURLConnection.html).
The mockwebserver dependency would only be needed in the test scope of this project.
Ignore my remark about OkHttp, that's just an opinion. 😉
|
Your change introduces a dependency to Apache's HTTP components which pulls in a couple of other dependencies. The code also requires at least Apache HTTP components 4.3+. Apache HTTP components is widely used and users use various versions. logstash-gelf would force its users to upgrade Apache HTTP components and this is not always easily possible. I'm also not sure whether to use the deprecated 4.0 API because it could be removed in future versions. Currently, I'd propose to use Java's HttpUrlConnection for data exchange and it supports also HTTPS out of the box. In general, this is a good approach. Configuring the URL via the Documentation for the particular senders is located within the Please use the formatter configuration to format your changes. |
|
Got it, I'll address the issues. On Sat, 20 Feb 2016, 19:53 Mark Paluch notifications@github.com wrote:
|
…URI if not present.
Conflicts: src/main/java/biz/paluch/logging/gelf/intern/sender/DefaultGelfSenderProvider.java src/main/java/biz/paluch/logging/gelf/intern/sender/GelfHTTPSender.java src/test/java/biz/paluch/logging/gelf/GelfUtilTest.java src/test/java/biz/paluch/logging/gelf/intern/sender/GelfHTTPSenderTest.java
logstash-gelf now supports sending log events using HTTP and HTTPS by using Java's HttpUrlConnection. Payload is sent according to the Graylog specification as JSON with prefixing additional fields with an underscore. Original pull request: #68.
|
Thanks. That's rebased, squashed and merged via 3cc5219. |
|
Damn, I notices I didn't remove the http-client dependency. I noticed but didn't have the time :( . Should I create a new PR? |
|
Thanks for the hint. I'll take care of it. |
Outline
Added HTTP Sender, for situations when the log manager is on a remote location or behind a proxy. Currently TLS and authentication is not supported, only log submission. TLS is the next step.
Motivation
The motivation is to use Graylog for remote logging, with Android devices, IoT and such.
Added configuration
As for the HTTP sender configuration, the host is the complete path to the log manager (i.e Graylog) endpoint, like
http://192.168.0.1:12201/gelf. If the port is not added to the host, the one supplied in the configuration file will be inserted into the host. I would gladly document this example, when I am informed where to put it :-) .Questions and issues
Regarding TLS and auth, additional configuration is needed (not sure which fields for now). I would like to consult with the author, which is the best way to do it? Should new configuration fields be added, available in all of the formats, hence, modifying all of the parsers, or some other way? I have noticed that the
GelfSenderConfigurationhas the methodMap<String, Object> getSpecificConfigurations();, and I suppose this is the way to go, based on the design. However, no example is provided how is this bound to the configuration files, and I'm not sure is it.Looking forward to continuing this,
Aleksandar