Use NullAway to prevent NPEs

(closes elastic#7) Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
felixbarny · Mar 14, 2018 · 32fa7e9 · 32fa7e9
1 parent c9bcec8
commit 32fa7e9
Show file tree

Hide file tree

Showing 9 changed files with 62 additions and 28 deletions.
diff --git a/apm-agent-benchmarks/src/main/java/co/elastic/apm/impl/AbstractReporterBenchmark.java b/apm-agent-benchmarks/src/main/java/co/elastic/apm/impl/AbstractReporterBenchmark.java
@@ -59,10 +59,9 @@ public void setUp() throws Exception {
             .withRuntime(new RuntimeInfo("Java", "9.0.4"))
             .withFramework(new Framework("Servlet API", "3.1"))
             .withLanguage(new Language("Java", "9.0.4"));
-        ProcessInfo process = new ProcessInfo()
+        ProcessInfo process = new ProcessInfo("/Library/Java/JavaVirtualMachines/jdk-9.0.4.jdk/Contents/Home/bin/java")
             .withPid(2103)
             .withPpid(403L)
-            .withTitle("/Library/Java/JavaVirtualMachines/jdk-9.0.4.jdk/Contents/Home/bin/java")
             .withArgv(Collections.singletonList("-javaagent:/path/to/elastic-apm-java.jar"));
         SystemInfo system = new SystemInfo("x86_64", "Felixs-MBP", "Mac OS X");
         ReporterConfiguration reporterConfiguration = new ReporterConfiguration();

diff --git a/apm-agent-core/src/main/java/co/elastic/apm/impl/payload/ProcessFactory.java b/apm-agent-core/src/main/java/co/elastic/apm/impl/payload/ProcessFactory.java
@@ -62,10 +62,9 @@ enum ForLegacyVM implements ProcessFactory {
 
         @Override
         public ProcessInfo getProcessInformation() {
-            ProcessInfo process = new ProcessInfo();
+            ProcessInfo process = new ProcessInfo(getTitle());
             process.withPid(getPid());
             process.withArgv(runtimeMXBean.getInputArguments());
-            process.withTitle(getTitle());
             return process;
         }
 
@@ -143,8 +142,8 @@ public static ProcessFactory make() {
          */
         @Override
         public ProcessInfo getProcessInformation() {
-            final ProcessInfo process = new ProcessInfo();
             ProcessHandle processHandle = (ProcessHandle) this.processHandle;
+            final ProcessInfo process = new ProcessInfo(processHandle.info().command().orElse(null));
             process.withPid(processHandle.pid());
             process.withPpid(processHandle.parent()
                 .map(new Function<ProcessHandle, Long>() {
@@ -163,7 +162,6 @@ public List<String> apply(String[] a) {
                     }
                 })
                 .orElse(null));
-            process.withTitle(processHandle.info().command().orElse(null));
 
             return process;
 

diff --git a/apm-agent-core/src/main/java/co/elastic/apm/impl/payload/ProcessInfo.java b/apm-agent-core/src/main/java/co/elastic/apm/impl/payload/ProcessInfo.java
@@ -7,6 +7,7 @@
 import org.apache.commons.lang.builder.HashCodeBuilder;
 import org.apache.commons.lang.builder.ToStringBuilder;
 
+import javax.annotation.Nullable;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -28,16 +29,21 @@ public class ProcessInfo {
     /**
      * Parent process ID of the service
      */
+    @Nullable
     @JsonProperty("ppid")
     private Long ppid;
     @JsonProperty("title")
-    private String title;
+    private final String title;
     /**
      * Command line arguments used to start this process
      */
     @JsonProperty("argv")
     private List<String> argv = new ArrayList<String>();
 
+    public ProcessInfo(String title) {
+        this.title = title;
+    }
+
     /**
      * Process ID of the service
      * (Required)
@@ -58,6 +64,7 @@ public ProcessInfo withPid(long pid) {
     /**
      * Parent process ID of the service
      */
+    @Nullable
     @JsonProperty("ppid")
     public Long getPpid() {
         return ppid;
@@ -76,15 +83,6 @@ public String getTitle() {
         return title;
     }
 
-    public void setTitle(String title) {
-        this.title = title;
-    }
-
-    public ProcessInfo withTitle(String title) {
-        this.title = title;
-        return this;
-    }
-
     /**
      * Command line arguments used to start this process
      */
@@ -103,12 +101,20 @@ public ProcessInfo withArgv(List<String> argv) {
 
     @Override
     public String toString() {
-        return new ToStringBuilder(this).append("pid", pid).append("ppid", ppid).append("title", title).append("argv", argv).toString();
+        return new ToStringBuilder(this)
+            .append("pid", pid)
+            .append("ppid", ppid)
+            .append("title", title)
+            .append("argv", argv).toString();
     }
 
     @Override
     public int hashCode() {
-        return new HashCodeBuilder().append(pid).append(title).append(argv).append(ppid).toHashCode();
+        return new HashCodeBuilder()
+            .append(pid)
+            .append(title)
+            .append(argv)
+            .append(ppid).toHashCode();
     }
 
     @Override
@@ -120,7 +126,11 @@ public boolean equals(Object other) {
             return false;
         }
         ProcessInfo rhs = ((ProcessInfo) other);
-        return new EqualsBuilder().append(pid, rhs.pid).append(title, rhs.title).append(argv, rhs.argv).append(ppid, rhs.ppid).isEquals();
+        return new EqualsBuilder()
+            .append(pid, rhs.pid)
+            .append(title, rhs.title)
+            .append(argv, rhs.argv)
+            .append(ppid, rhs.ppid).isEquals();
     }
 
 }
diff --git a/apm-agent-core/src/main/java/co/elastic/apm/impl/payload/Service.java b/apm-agent-core/src/main/java/co/elastic/apm/impl/payload/Service.java
@@ -1,4 +1,3 @@
-
 package co.elastic.apm.impl.payload;
 
 import com.fasterxml.jackson.annotation.JsonInclude;
@@ -7,56 +6,66 @@
 import org.apache.commons.lang.builder.HashCodeBuilder;
 import org.apache.commons.lang.builder.ToStringBuilder;
 
+import javax.annotation.Nullable;
+
 
 /**
  * Information about the instrumented Service
  */
 @JsonInclude(JsonInclude.Include.NON_NULL)
-// TODO: make immutable
+// TODO reduce usage of @Nullable
 public class Service {
 
     /**
      * Name and version of the Elastic APM agent
      * (Required)
      */
+    @Nullable
     @JsonProperty("agent")
     private Agent agent;
     /**
      * Name and version of the web framework used
      */
+    @Nullable
     @JsonProperty("framework")
     private Framework framework;
     /**
      * Name and version of the programming language used
      */
+    @Nullable
     @JsonProperty("language")
     private Language language;
     /**
      * Immutable name of the service emitting this event
      * (Required)
      */
+    @Nullable
     @JsonProperty("name")
     private String name;
     /**
      * Environment name of the service, e.g. "production" or "staging"
      */
+    @Nullable
     @JsonProperty("environment")
     private String environment;
     /**
      * Name and version of the language runtime running this service
      */
+    @Nullable
     @JsonProperty("runtime")
     private RuntimeInfo runtime;
     /**
      * Version of the service emitting this event
      */
+    @Nullable
     @JsonProperty("version")
     private String version;
 
     /**
      * Name and version of the Elastic APM agent
      * (Required)
      */
+    @Nullable
     @JsonProperty("agent")
     public Agent getAgent() {
         return agent;
@@ -74,6 +83,7 @@ public Service withAgent(Agent agent) {
     /**
      * Name and version of the web framework used
      */
+    @Nullable
     @JsonProperty("framework")
     public Framework getFramework() {
         return framework;
@@ -90,6 +100,7 @@ public Service withFramework(Framework framework) {
     /**
      * Name and version of the programming language used
      */
+    @Nullable
     @JsonProperty("language")
     public Language getLanguage() {
         return language;
@@ -107,6 +118,7 @@ public Service withLanguage(Language language) {
      * Immutable name of the service emitting this event
      * (Required)
      */
+    @Nullable
     @JsonProperty("name")
     public String getName() {
         return name;
@@ -124,6 +136,7 @@ public Service withName(String name) {
     /**
      * Environment name of the service, e.g. "production" or "staging"
      */
+    @Nullable
     @JsonProperty("environment")
     public String getEnvironment() {
         return environment;
@@ -140,6 +153,7 @@ public Service withEnvironment(String environment) {
     /**
      * Name and version of the language runtime running this service
      */
+    @Nullable
     @JsonProperty("runtime")
     public RuntimeInfo getRuntime() {
         return runtime;
@@ -156,6 +170,7 @@ public Service withRuntime(RuntimeInfo runtime) {
     /**
      * Version of the service emitting this event
      */
+    @Nullable
     @JsonProperty("version")
     public String getVersion() {
         return version;

diff --git a/apm-agent-core/src/main/java/co/elastic/apm/impl/transaction/Span.java b/apm-agent-core/src/main/java/co/elastic/apm/impl/transaction/Span.java
@@ -11,6 +11,7 @@
 import org.apache.commons.lang.builder.ToStringBuilder;
 
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -69,7 +70,7 @@ public class Span implements Recyclable, co.elastic.apm.api.Span {
     @JsonProperty("type")
     private String type;
 
-    public Span start(ElasticApmTracer tracer, Transaction transaction, Span span, long nanoTime, boolean dropped) {
+    public Span start(ElasticApmTracer tracer, Transaction transaction, @Nullable Span span, long nanoTime, boolean dropped) {
         this.tracer = tracer;
         this.id.setToRandomValue();
         if (span != null) {

diff --git a/...gent-core/src/test/java/co/elastic/apm/impl/payload/TransactionPayloadJsonSchemaTest.java b/...gent-core/src/test/java/co/elastic/apm/impl/payload/TransactionPayloadJsonSchemaTest.java
@@ -28,7 +28,7 @@ void setUp() {
     private TransactionPayload createPayloadWithRequiredValues() {
         Service service = new Service().withAgent(new Agent("name", "version")).withName("name");
         SystemInfo system = new SystemInfo("", "", "");
-        return new TransactionPayload(new ProcessInfo(), service, system);
+        return new TransactionPayload(new ProcessInfo("title"), service, system);
     }
 
     private Transaction createTransactionWithRequiredValues() {

diff --git a/apm-agent-core/src/test/java/co/elastic/apm/report/ApmServerReporterIntegrationTest.java b/apm-agent-core/src/test/java/co/elastic/apm/report/ApmServerReporterIntegrationTest.java
@@ -1,10 +1,10 @@
 package co.elastic.apm.report;
 
 import co.elastic.apm.impl.error.ErrorCapture;
-import co.elastic.apm.impl.transaction.Transaction;
 import co.elastic.apm.impl.payload.ProcessInfo;
 import co.elastic.apm.impl.payload.Service;
 import co.elastic.apm.impl.payload.SystemInfo;
+import co.elastic.apm.impl.transaction.Transaction;
 import co.elastic.apm.report.serialize.JacksonPayloadSerializer;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.module.afterburner.AfterburnerModule;
@@ -59,7 +59,7 @@ void setUp() {
         when(reporterConfiguration.getServerUrl()).thenReturn("http://localhost:" + port);
         payloadSender = new ApmServerHttpPayloadSender(new OkHttpClient(), new JacksonPayloadSerializer(objectMapper), reporterConfiguration);
         SystemInfo system = new SystemInfo("x64", "localhost", "platform");
-        reporter = new ApmServerReporter(new Service(), new ProcessInfo(), system, payloadSender, false, reporterConfiguration);
+        reporter = new ApmServerReporter(new Service(), new ProcessInfo("title"), system, payloadSender, false, reporterConfiguration);
     }
 
     @Test

diff --git a/apm-agent-core/src/test/java/co/elastic/apm/report/ApmServerReporterTest.java b/apm-agent-core/src/test/java/co/elastic/apm/report/ApmServerReporterTest.java
@@ -28,7 +28,7 @@ void setUp() {
         when(reporterConfiguration.getMaxQueueSize()).thenReturn(2);
         SystemInfo system = new SystemInfo("x64", "localhost", "platform");
         payloadSender = mock(PayloadSender.class);
-        reporter = new ApmServerReporter(new Service(), new ProcessInfo(), system, payloadSender, true, reporterConfiguration);
+        reporter = new ApmServerReporter(new Service(), new ProcessInfo("title"), system, payloadSender, true, reporterConfiguration);
     }
 
     @Test

diff --git a/pom.xml b/pom.xml
@@ -55,6 +55,17 @@
                     <testSource>${maven.compiler.testTarget}</testSource>
                     <testTarget>${maven.compiler.testTarget}</testTarget>
                     <encoding>UTF-8</encoding>
+                    <annotationProcessorPaths>
+                        <path>
+                            <groupId>com.uber.nullaway</groupId>
+                            <artifactId>nullaway</artifactId>
+                            <version>0.3.0</version>
+                        </path>
+                    </annotationProcessorPaths>
+                    <compilerArgs>
+                        <arg>-Xep:NullAway:ERROR</arg>
+                        <arg>-XepOpt:NullAway:AnnotatedPackages=co.elastic.apm</arg>
+                    </compilerArgs>
                 </configuration>
                 <dependencies>
                     <dependency>
@@ -158,8 +169,8 @@
         </dependency>
         <dependency>
             <groupId>com.google.code.findbugs</groupId>
-            <artifactId>annotations</artifactId>
-            <version>3.0.1</version>
+            <artifactId>jsr305</artifactId>
+            <version>3.0.2</version>
             <scope>compile</scope>
         </dependency>
         <dependency>