Gelf writer #590
Gelf writer #590
Conversation
|
The current build problems doesn't seem to relate to the new writer. Could you please look at them? |
|
The build is failing because mutation testing is a bit low (30%). The default is 100% (which you are not expected to achieve). Have a look at other modules to see how to decrease that threshold. |
| <artifactId>mockito-core</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
The need to use powermock is a design smell in my book (I have not yet looked at the rest of the code, so there might be a good reason to use it here).
There was a problem hiding this comment.
Oh, this actually is a leftover. I'm not using Powermock anymore and will remove it.
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class GelfWriter extends BaseOutputWriter { |
There was a problem hiding this comment.
Splitting this class with OutputWriterFactory and an OutputWriter would split the responsibilities of creating the writer vs running the writer. Have a look at GraphiteWriter2 / GraphiteWriterFactory for an example. In particular, it would allow you to get rid of the mock transport in the writer and inject it during tests.
There was a problem hiding this comment.
Now that the responsibilities are split, this class should implement directly OutputWriter or extend OutputWriterAdapter.This would allow to drop a few unused methods (validateSetup() for one).
There was a problem hiding this comment.
But the OutputWriter interface includes a validateSetup() method. If I implement that, I have to implement lots of other methods like start() and close, which BaseOutputWriter already does. Are you sure about this one?
There was a problem hiding this comment.
OutputWriterAdapter provides default implementations for those methods.
| messageBuilder.message(message); | ||
| messageBuilder.fullMessage(message); | ||
|
|
||
| log.debug( |
There was a problem hiding this comment.
Don't use String.format() in log messages, use the SLF4J placeholders ({}).
|
|
||
| @Before | ||
| public void setUp() throws Exception { | ||
| this.result = new Result(1, "attributeName", "className", "objDomain", "classNameAlias", "typeName", ImmutableMap |
There was a problem hiding this comment.
There are a bunch of ResultFixtures already available that you could reuse.
| null, | ||
| null | ||
| ); | ||
| } catch (final NullPointerException e) { |
There was a problem hiding this comment.
I'm not sure that catching NPE here adds a lot of clarity. If you really want to do it, please rethrow a new NPE, not a RuntimeException (we are clearly in a case of a null pointer exception).
There was a problem hiding this comment.
I referenced the Elastic tests here, which do the same, but I will change that.
| } | ||
| gelfWriter.start(); | ||
| gelfWriter.doWrite(dummyServer(), dummyQuery(), ImmutableList.<Result>of(result)); | ||
| Assert.assertTrue( |
There was a problem hiding this comment.
AssertJ would make those assertions easier to read (assertThat(MockGelfTransport.calls).size().isNotZero()).
pom.xml
Outdated
| @@ -110,15 +112,17 @@ | |||
|
|
|||
| <scm> | |||
| <connection>scm:git:git@github.com:jmxtrans/jmxtrans.git</connection> | |||
| <developerConnection>scm:git:git@github.com:jmxtrans/jmxtrans.git</developerConnection> | |||
| <developerConnection>scm:git:git@github.com:jmxtrans/jmxtrans.git | |||
| </developerConnection> | |||
There was a problem hiding this comment.
I don't like those new lines... probably a too eager reformat?
|
|
||
| log.debug( | ||
| "Sending GELF message: {}", | ||
| messageBuilder.build().toString() |
There was a problem hiding this comment.
It looks like you are building the message twice. Storing it in a local variable would be nicer (I do expect that building this message is cheap, so this is not much of an optimization, feel free to ignore).
| public GelfWriter( | ||
| ImmutableList<String> typeNames, | ||
| boolean booleanAsNumber, | ||
| Boolean debugEnabled, |
There was a problem hiding this comment.
It looks like debugEnabled isn't used anymore.
There was a problem hiding this comment.
No, I implemented that, because I thought it would be needed. BaseOutputWriter has that in the constructor. I don't need it.
There was a problem hiding this comment.
Removed it now. 鉁旓笍
| import java.io.File; | ||
| import java.util.Map; | ||
|
|
||
| public class GelfWriterFactory implements OutputWriterFactory{ |
There was a problem hiding this comment.
This class should implement a meaningfull equals and hashCode. Lombok can help you here.
There was a problem hiding this comment.
Hm. Why? It doesn't seem to be a child of Comparable. My IntelliJ usually recommends and creates that, if the class is a Comparable. Are WriterFactories compared somewhere in the code?
There was a problem hiding this comment.
Nope, they are not Comparable, but they are deduplicated using a HashMap, which requires a meaningfull equals and hashCode. Don't look at that code to closely, it is ugly :) here lies dragons...
There was a problem hiding this comment.
Okay, will do then. 馃槈
| private final Map<String, Object> additionalFields; | ||
| private final GelfTransport gelfTransport; | ||
|
|
||
| public GelfWriter( |
There was a problem hiding this comment.
Not absolutely required, but it would be nice if this class implements a meaningful equals and hashCode. Lombok can help.
|
|
||
| @Before | ||
| public void setUp() throws Exception { | ||
| this.result = ResultFixtures.numericResult(); |
There was a problem hiding this comment.
This could probably be inlined everywhere.
| import static org.mockito.Mockito.times; | ||
| import static org.mockito.Mockito.verify; | ||
|
|
||
| public class GelfWriterTests { |
There was a problem hiding this comment.
The tests in this class have mostly no assertions. This explains the fairly low mutation test coverage. It would be nice to check a bit more that not only the code does not throw any exceptions, but that is actually does what it is supposed to do. It might also make sense to split test for the Writer and tests for the Factory.
|
|
||
| final Map<String, Object> additionalFields = new HashMap<>(); | ||
|
|
||
| additionalFields.put("test", "test"); |
There was a problem hiding this comment.
Adding additional fields to test that they are working correctly is good! Testing that they are actually sent would be much better. You should probably create a mock transport, capture the message and do a few assertions on it. The same kind of additional assertions could be added to most of those tests.
Side note: ImmutableMap.of() would be a much shorter notation to declare those additionalFields.
There was a problem hiding this comment.
|
Note that you can sort the pom.xml with |
| <dependency> | ||
| <groupId>org.graylog2</groupId> | ||
| <artifactId>gelfclient</artifactId> | ||
| <version>1.4.0</version> |
There was a problem hiding this comment.
Could you move version declaration to the parent pom? (in the dependencyManagement section)
|
|
||
| private final ImmutableList<String> typeNames; | ||
| private final boolean booleanAsNumber; | ||
| private final Boolean debugEnabled; |
There was a problem hiding this comment.
debugEnabled and settings don't seem to be used. They should be removed.
|
|
||
| public GelfWriterFactory( | ||
| @JsonProperty("typeNames") final ImmutableList<String> typeNames, | ||
| @JsonProperty("booleanAsNumber") final boolean booleanAsNumber, |
There was a problem hiding this comment.
booleanAsNumber should be used by wrapping the generated OutputWriter in the appropriate transformer. There are some other OutputWriter that already use that.
There was a problem hiding this comment.
I removed it as I was not using it. (Or should I use it?)
There was a problem hiding this comment.
It looks like booleanAsNumber is still there. I'm not sure about your use case, and converting boleans to numbers probably does not make all that much sense for GELF. So probably remove it.
| ); | ||
| } | ||
|
|
||
| public GelfConfiguration getGelfConfiguration() { |
There was a problem hiding this comment.
This shoudl probably be marked as @VisibleForTesting.
| assertThat( | ||
| gelfWriterFactory.getGelfConfiguration() | ||
| ).is( | ||
| new Condition<GelfConfiguration>() { |
There was a problem hiding this comment.
I'm really not sure that using a Condition is more readable than testing individual fields. I would go for a series of:
assertThat(gelfConfiguration.getQueueSize()).isEqualsTo(10)
...
There was a problem hiding this comment.
Hm. I found the (rather long) series of assertThat not really readable. Maybe move it to another class as I did with GelfDefaultConfigurationCondition?
There was a problem hiding this comment.
I would argue that the current version isn't much readable either. I don't think that extracting assertions to another class would help readability much, as assertions are very much part of your test. In any case, that's really a minor style issue...
There was a problem hiding this comment.
You're right, actually. I noticed that the output given when using Conditions isn't really helping when trying to find the bug. I'm moving to assertThat-lines.
|
|
||
| final GelfTransport gelfTransport = Mockito.mock(GelfTcpTransport.class); | ||
|
|
||
| doNothing().when(gelfTransport).send( |
There was a problem hiding this comment.
Using a captor instead of an argument matcher would allow a more readable test IMHO.
|
@gehel I think, I fixed all your requested changes and the build is successful. |
|
|
||
| private static final Logger log = LoggerFactory.getLogger(GelfWriter.class); | ||
|
|
||
| private final Map<String, Object> additionalFields; |
There was a problem hiding this comment.
Really minor... This map should probably be an ImmutableMap<String, String>. As it is coming from json, I doubt that values of anything else than a String make much sense.
There was a problem hiding this comment.
ImmutableMap maybe, but <String, Object> is correct, because JSON could contain integers or booleans.
| ArgumentCaptor<GelfMessage> messageArgumentCaptor; | ||
|
|
||
| @Before | ||
| public void setUp() throws Exception { |
There was a problem hiding this comment.
This could be replaced by @RunWith(MockitoJUnitRunner.class) (which I find more readable, but that's personal style).
|
|
||
| final GelfTransport | ||
| gelfTransport = | ||
| Mockito.mock(GelfTcpTransport.class); |
There was a problem hiding this comment.
Since you are already using @Captor, using @Mock would be nicer. Or at least have a static import for Mockito.mock().
|
|
||
| gelfWriter = new GelfWriter( | ||
| typenames, | ||
| ImmutableMap.of("test", (Object) "test"), |
There was a problem hiding this comment.
Having test values a bit more descriptive than "test" would help document what is going on. Could you provide an example of something you would actually use? Or at least a "additional_test_field" / "additional_test_value"?
| gelfWriter.start(); | ||
| gelfWriter.doWrite(dummyServer(), | ||
| dummyQuery(), | ||
| ImmutableList.of(ResultFixtures.numericResult())); |
There was a problem hiding this comment.
There is a singleNumericResult fixture which does exactly what you are doing here.
|
This is looking really good! I have a few more comments (I am a pain in the ..., yes I know). Feel free to let me know if I'm too much of a pain. Mutation coverage is still on the low side. But most of the nominal paths are correctly asserted, so I'm fine with merging as is. |
|
@gehel I will look into your suggestions tomorrow, please wait with merging. Thanks for all the good tips and suggestions. |
|
Done. |
|
The |
|
Damn. Corrected that as well. |
|
Looks good! Merging! Thanks a lot for the efforts! |
|
You're welcome. Added wiki documentation as well. Can you tell me, when this will be available in an official release? |
|
@gehel The GELF-Writer doesn't seem to be in 266-SNAPSHOT. Any clues why? |
|
The jar itself seems to be published (https://oss.sonatype.org/content/repositories/snapshots/org/jmxtrans/jmxtrans-output-gelf/266-SNAPSHOT/). I have not checked the main ueber jar, but I suspect just some delay with sonatype. If that's not the case, let me know, I'll investigate some more. I can make a release tonight... |
|
Release in progress: https://travis-ci.org/jmxtrans/jmxtrans/builds/255795067 (I'll check on it tomorrow, but it should just complete with no problem). |
|
I just downloaded the latest nightly all.jar from https://oss.sonatype.org/content/repositories/snapshots/org/jmxtrans/jmxtrans/266-SNAPSHOT/ and it was missing there. Thanks. |
Fixes #589
Have fun. 馃槈