Better shutdown of Elasticsearch client (#493)

* Add ErrorProne build checks * Fix ES Client potential leak of http connections * revert okhttp lib version
jenkinsci · Aug 18, 2022 · fe664cf · fe664cf
1 parent f40868c
commit fe664cf
Show file tree

Hide file tree

Showing 19 changed files with 129 additions and 164 deletions.
diff --git a/pom.xml b/pom.xml
@@ -25,7 +25,8 @@
         <opentelemetry.version>1.17.0</opentelemetry.version>
         <opentelemetry-alpha.version>1.17.0-alpha</opentelemetry-alpha.version>
         <useBeta>true</useBeta>
-        <elasticstack.version>8.3.1</elasticstack.version>
+        <elasticstack.version>8.3.3</elasticstack.version>
+        <error-prone.version>2.15.0</error-prone.version>
     </properties>
     <name>OpenTelemetry Plugin</name>
     <description>Monitor, troubleshoot and observe Jenkins with OpenTelemetry.
@@ -163,7 +164,7 @@
         <dependency>
             <groupId>io.jenkins.plugins</groupId>
             <artifactId>okhttp-api</artifactId>
-            <version>4.9.3-108.v9a_36cd2c64b_2</version>
+            <version>4.9.3-105.vb96869f8ac3a</version>
         </dependency>
         <dependency> <!-- see maskClasses config -->
             <groupId>com.google.guava</groupId>
@@ -179,7 +180,7 @@
         <dependency>
             <groupId>com.google.errorprone</groupId>
             <artifactId>error_prone_annotations</artifactId>
-            <version>2.14.0</version>
+            <version>${error-prone.version}</version>
         </dependency>
         <dependency>
             <groupId>com.google.code.findbugs</groupId>
@@ -482,17 +483,6 @@
                     </loggers>
                 </configuration>
             </plugin>
-            <plugin>
-                <groupId>org.apache.maven.plugins</groupId>
-                <artifactId>maven-compiler-plugin</artifactId>
-                <version>3.8.1</version>
-                <configuration>
-                    <!--
-                    https://stackoverflow.com/questions/49939398/maven-compiler-plugin-null-pointer
-                    -->
-                    <forceJavacCompilerUse>true</forceJavacCompilerUse>
-                </configuration>
-            </plugin>
         </plugins>
     </build>
 
@@ -510,6 +500,37 @@
         <url>https://github.com/${gitHubRepo}</url>
         <tag>${scmTag}</tag>
     </scm>
+    <profiles>
+        <profile>
+            <!--
+            Error Prone doesn't work to produce Jenkins plugins but it's useful for detecting bugs, particularly bugs of resource not closed
+            Waiting for Jenkins Plugin pom to include Error Prone, we keep the Error Prone activation config in a Maven profile
+            -->
+            <id>error-prone-check</id>
+            <build>
+                <plugins>
+                    <plugin>
+                        <groupId>org.apache.maven.plugins</groupId>
+                        <artifactId>maven-compiler-plugin</artifactId>
+                        <configuration>
+                            <encoding>UTF-8</encoding>
+                            <compilerArgs>
+                                <arg>-XDcompilePolicy=simple</arg>
+                                <arg>-Xplugin:ErrorProne</arg>
+                            </compilerArgs>
+                            <annotationProcessorPaths>
+                                <path>
+                                    <groupId>com.google.errorprone</groupId>
+                                    <artifactId>error_prone_core</artifactId>
+                                    <version>${error-prone.version}</version>
+                                </path>
+                            </annotationProcessorPaths>
+                        </configuration>
+                    </plugin>
+                </plugins>
+            </build>
+        </profile>
+    </profiles>
     <contributors>
         <contributor>
             <!-- jenkins id cleclerc -->

diff --git a/src/main/java/io/jenkins/plugins/opentelemetry/JenkinsOpenTelemetryPluginConfiguration.java b/src/main/java/io/jenkins/plugins/opentelemetry/JenkinsOpenTelemetryPluginConfiguration.java
@@ -6,6 +6,7 @@
 package io.jenkins.plugins.opentelemetry;
 
 import com.google.common.base.Strings;
+import com.google.errorprone.annotations.MustBeClosed;
 import groovy.text.GStringTemplateEngine;
 import hudson.Extension;
 import hudson.PluginWrapper;
@@ -49,7 +50,6 @@
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
-import javax.annotation.PostConstruct;
 import javax.annotation.PreDestroy;
 import javax.annotation.concurrent.Immutable;
 import javax.inject.Inject;
@@ -208,6 +208,7 @@ public OpenTelemetryConfiguration toOpenTelemetryConfiguration() {
      * JCasC configuration happens during `SYSTEM_CONFIG_ADAPTED` (see `io.jenkins.plugins.casc.ConfigurationAsCode#init()`)
      */
     @Initializer(after = InitMilestone.SYSTEM_CONFIG_ADAPTED, before = InitMilestone.JOB_LOADED)
+    @SuppressWarnings("MustBeClosedChecker")
     public void initializeOpenTelemetry() {
         LOGGER.log(Level.FINE, "Initialize Jenkins OpenTelemetry Plugin...");
         OpenTelemetryConfiguration newOpenTelemetryConfiguration = toOpenTelemetryConfiguration();
@@ -280,7 +281,7 @@ public void setTrustedCertificatesPem(String trustedCertificatesPem) {
 
     @DataBoundSetter
     public void setObservabilityBackends(List<ObservabilityBackend> observabilityBackends) {
-        this.observabilityBackends = Optional.of(observabilityBackends).orElse(Collections.emptyList());
+        this.observabilityBackends = observabilityBackends == null ? Collections.emptyList() : observabilityBackends;
     }
 
     @Nonnull
@@ -551,11 +552,13 @@ public LogStorageRetriever getLogStorageRetriever() {
     }
 
     @Nonnull
+    @MustBeClosed
+    @SuppressWarnings("MustBeClosedChecker") // false positive invoking backend.getLogStorageRetriever(templateBindingsProvider)
     private LogStorageRetriever resolveLogStorageRetriever() {
         LogStorageRetriever logStorageRetriever = null;
         for (ObservabilityBackend backend : getObservabilityBackends()) {
             ObservabilityBackend templateBindingsProvider = backend; // TODO make JenkinsOpenTelemetryPluginConfiguration a templateBindingsProvider
-            logStorageRetriever = backend.getLogStorageRetriever(templateBindingsProvider);
+            logStorageRetriever = backend.newLogStorageRetriever(templateBindingsProvider);
             if (logStorageRetriever != null) {
                 break;
             }
@@ -644,10 +647,10 @@ public FormValidation doCheckExportOtelConfigurationAsEnvironmentVariables(@Quer
     }
 
     @PreDestroy
-    public void preDestroy() throws IOException {
-        if (logStorageRetriever != null && logStorageRetriever instanceof Closeable) {
+    public void preDestroy() throws Exception {
+        if (logStorageRetriever != null) {
             LOGGER.log(Level.FINE, () -> "Close " + logStorageRetriever + "...");
-            ((Closeable) logStorageRetriever).close();
+            logStorageRetriever.close();
         }
     }
 

diff --git a/src/main/java/io/jenkins/plugins/opentelemetry/backend/ElasticBackend.java b/src/main/java/io/jenkins/plugins/opentelemetry/backend/ElasticBackend.java
@@ -5,6 +5,7 @@
 
 package io.jenkins.plugins.opentelemetry.backend;
 
+import com.google.errorprone.annotations.MustBeClosed;
 import hudson.Extension;
 import hudson.util.FormValidation;
 import io.jenkins.plugins.opentelemetry.TemplateBindingsProvider;
@@ -17,7 +18,6 @@
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
 import org.kohsuke.stapler.QueryParameter;
-import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
@@ -26,6 +26,7 @@
 import java.net.URL;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -185,11 +186,20 @@ public void setElasticLogsBackend(ElasticLogsBackend elasticLogsBackend) {
 
     @Nullable
     @Override
-    public LogStorageRetriever getLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
+    @MustBeClosed
+    public LogStorageRetriever newLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
         if (elasticLogsBackend == null) {
             return null;
         } else {
-            return elasticLogsBackend.getLogStorageRetriever(templateBindingsProvider);
+            return elasticLogsBackend.newLogStorageRetriever(templateBindingsProvider);
+        }
+    }
+
+    public Map<String, String> getOtelConfigurationProperties() {
+        if (elasticLogsBackend == null) {
+            return Collections.emptyMap();
+        } else {
+            return Collections.singletonMap("otel.logs.exporter", "otlp");
         }
     }
 

diff --git a/src/main/java/io/jenkins/plugins/opentelemetry/backend/ObservabilityBackend.java b/src/main/java/io/jenkins/plugins/opentelemetry/backend/ObservabilityBackend.java
@@ -7,6 +7,7 @@
 
 import com.google.common.base.Strings;
 import com.google.common.collect.ComparisonChain;
+import com.google.errorprone.annotations.MustBeClosed;
 import groovy.lang.MissingPropertyException;
 import groovy.text.GStringTemplateEngine;
 import groovy.text.Template;
@@ -66,7 +67,8 @@ public abstract class ObservabilityBackend implements Describable<ObservabilityB
      * @return the {@link LogStorageRetriever} of this backend if the backend is configured to retrieve logs. {@code null} otherwise.
      */
     @CheckForNull
-    public LogStorageRetriever getLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
+    @MustBeClosed
+    public LogStorageRetriever newLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
         return null;
     }
 
@@ -161,11 +163,6 @@ public static DescriptorExtensionList<ObservabilityBackend, ObservabilityBackend
      */
     @Nonnull
     public Map<String, String> getOtelConfigurationProperties() {
-        LogStorageRetriever logStorageRetriever = getLogStorageRetriever(TemplateBindingsProvider.empty());
-        if (logStorageRetriever != null) {
-            LOGGER.log(Level.FINE, () -> "Configure OpenTelemetry SDK to export logs");
-            return Collections.singletonMap("otel.logs.exporter", "otlp");
-        }
         return Collections.emptyMap();
     }
 

diff --git a/src/main/java/io/jenkins/plugins/opentelemetry/backend/custom/CustomLogStorageRetriever.java b/src/main/java/io/jenkins/plugins/opentelemetry/backend/custom/CustomLogStorageRetriever.java
@@ -64,4 +64,9 @@ public String toString() {
             "urlTemplate=" + buildLogsVisualizationUrlTemplate +
             '}';
     }
+
+    @Override
+    public void close() throws Exception {
+
+    }
 }
diff --git a/src/main/java/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticLogsBackend.java b/src/main/java/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticLogsBackend.java
@@ -5,6 +5,7 @@
 
 package io.jenkins.plugins.opentelemetry.backend.elastic;
 
+import com.google.errorprone.annotations.MustBeClosed;
 import groovy.text.GStringTemplateEngine;
 import groovy.text.Template;
 import hudson.DescriptorExtensionList;
@@ -19,7 +20,6 @@
 import org.apache.commons.lang.StringUtils;
 
 import javax.annotation.CheckForNull;
-import javax.annotation.Nonnull;
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
@@ -36,7 +36,8 @@ public abstract class ElasticLogsBackend extends AbstractDescribableImpl<Elastic
      * Returns {@code null} if the backend is not capable of retrieving logs(ie the {@link NoElasticLogsBackend}
      */
     @CheckForNull
-    public abstract LogStorageRetriever getLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider);
+    @MustBeClosed
+    public abstract LogStorageRetriever newLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider);
 
     public Template getBuildLogsVisualizationMessageTemplate() {
         try {

diff --git a/...ins/plugins/opentelemetry/backend/elastic/ElasticLogsBackendWithJenkinsVisualization.java b/...ins/plugins/opentelemetry/backend/elastic/ElasticLogsBackendWithJenkinsVisualization.java
@@ -7,6 +7,7 @@
 
 import com.cloudbees.plugins.credentials.common.StandardListBoxModel;
 import com.cloudbees.plugins.credentials.common.StandardUsernameCredentials;
+import com.google.errorprone.annotations.MustBeClosed;
 import groovy.text.Template;
 import hudson.Extension;
 import hudson.Util;
@@ -43,7 +44,8 @@ public ElasticLogsBackendWithJenkinsVisualization() {
     }
 
     @Override
-    public LogStorageRetriever getLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
+    @MustBeClosed
+    public LogStorageRetriever newLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
         Template buildLogsVisualizationUrlTemplate = getBuildLogsVisualizationUrlTemplate();
         if (StringUtils.isBlank(elasticsearchUrl)) {
             return null; // FIXME handle case where this logs retriever is miss configured lacking of an Elasticsearch URL. We should use the rendering  ElasticLogsBackendWithVisualizationOnlyThroughKibana
@@ -162,14 +164,14 @@ public FormValidation doValidate(@QueryParameter String elasticsearchUrl, @Query
                 return elasticsearchUrlValidation;
             }
 
-            try {
-                Credentials credentials = new JenkinsCredentialsToApacheHttpCredentialsAdapter(() -> elasticsearchCredentialsId);
-                ElasticsearchLogStorageRetriever elasticsearchLogStorageRetriever = new ElasticsearchLogStorageRetriever(
+            Credentials credentials = new JenkinsCredentialsToApacheHttpCredentialsAdapter(() -> elasticsearchCredentialsId);
+            // TODO cleanup code, we shouldn't have to instantiate the ElasticsearchLogStorageRetriever to check the proper configuration of the access to Elasticsearch
+            try (ElasticsearchLogStorageRetriever elasticsearchLogStorageRetriever = new ElasticsearchLogStorageRetriever(
                     elasticsearchUrl,
                     disableSslVerifications,
                     credentials,
-                    ObservabilityBackend.ERROR_TEMPLATE, // TODO cleanup code, we shouldn't have to instantiate the ElasticsearchLogStorageRetriever to check the proper configuration of the access to Elasticsearch
-                    TemplateBindingsProvider.empty());
+                    ObservabilityBackend.ERROR_TEMPLATE,
+                    TemplateBindingsProvider.empty())) {
 
                 return FormValidation.aggregate(elasticsearchLogStorageRetriever.checkElasticsearchSetup());
             } catch (NoSuchElementException e) {

diff --git a/.../plugins/opentelemetry/backend/elastic/ElasticLogsBackendWithoutJenkinsVisualization.java b/.../plugins/opentelemetry/backend/elastic/ElasticLogsBackendWithoutJenkinsVisualization.java
@@ -5,6 +5,7 @@
 
 package io.jenkins.plugins.opentelemetry.backend.elastic;
 
+import com.google.errorprone.annotations.MustBeClosed;
 import hudson.Extension;
 import io.jenkins.plugins.opentelemetry.TemplateBindingsProvider;
 import io.jenkins.plugins.opentelemetry.backend.custom.CustomLogStorageRetriever;
@@ -19,7 +20,8 @@ public ElasticLogsBackendWithoutJenkinsVisualization() {
     }
 
     @Override
-    public LogStorageRetriever getLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
+    @MustBeClosed
+    public LogStorageRetriever newLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
         return new CustomLogStorageRetriever(getBuildLogsVisualizationUrlTemplate(), templateBindingsProvider);
     }
 

diff --git a/...va/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticsearchLogStorageRetriever.java b/...va/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticsearchLogStorageRetriever.java
@@ -15,6 +15,7 @@
 import co.elastic.clients.elasticsearch.indices.ElasticsearchIndicesClient;
 import co.elastic.clients.json.jackson.JacksonJsonpMapper;
 import co.elastic.clients.transport.rest_client.RestClientTransport;
+import com.google.errorprone.annotations.MustBeClosed;
 import groovy.text.Template;
 import hudson.util.FormValidation;
 import io.jenkins.plugins.opentelemetry.OpenTelemetrySdkProvider;
@@ -79,7 +80,8 @@ public class ElasticsearchLogStorageRetriever implements LogStorageRetriever, Cl
     final Credentials elasticsearchCredentials;
     @Nonnull
     final String elasticsearchUrl;
-
+    @Nonnull
+    final RestClient restClient;
     @Nonnull
     final RestClientTransport elasticsearchTransport;
     @Nonnull
@@ -90,6 +92,7 @@ public class ElasticsearchLogStorageRetriever implements LogStorageRetriever, Cl
     /**
      * TODO verify username:password auth vs apiKey auth
      */
+    @MustBeClosed
     public ElasticsearchLogStorageRetriever(
         @Nonnull String elasticsearchUrl, boolean disableSslVerifications,
         @Nonnull Credentials elasticsearchCredentials,
@@ -104,7 +107,7 @@ public ElasticsearchLogStorageRetriever(
         BasicCredentialsProvider credentialsProvider = new BasicCredentialsProvider();
         credentialsProvider.setCredentials(AuthScope.ANY, elasticsearchCredentials);
 
-        RestClient restClient = RestClient.builder(HttpHost.create(elasticsearchUrl))
+        this.restClient = RestClient.builder(HttpHost.create(elasticsearchUrl))
             .setHttpClientConfigCallback(httpClientBuilder -> {
                 httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider);
                 if (disableSslVerifications) {
@@ -337,13 +340,13 @@ protected static String prettyPrintPhaseRetentionPolicy(Phase phase, String phas
     public void close() throws IOException {
         logger.log(Level.FINE, () -> "Shutdown Elasticsearch client...");
         this.elasticsearchTransport.close();
+        this.restClient.close();
     }
 
     @Override
     public String toString() {
         return "ElasticsearchLogStorageRetriever{" +
-            "buildLogsVisualizationUrlTemplate=" + buildLogsVisualizationUrlTemplate +
-            ", templateBindingsProvider=" + templateBindingsProvider +
+            "elasticsearchUrl=" + elasticsearchUrl +
             '}';
     }
 

diff --git a/src/main/java/io/jenkins/plugins/opentelemetry/backend/elastic/NoElasticLogsBackend.java b/src/main/java/io/jenkins/plugins/opentelemetry/backend/elastic/NoElasticLogsBackend.java
@@ -17,7 +17,7 @@ public NoElasticLogsBackend() {
     }
 
     @Override
-    public LogStorageRetriever getLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
+    public LogStorageRetriever newLogStorageRetriever(TemplateBindingsProvider templateBindingsProvider) {
         return null;
     }