Config: allow per-check timeout configuration Introduce healthcheck.config and allow global and check-specific timeout settings. Change-Id: I800908168ec232f98282fccf42216876b420be5f
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java new file mode 100644 index 0000000..b1fe43d --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java
@@ -0,0 +1,60 @@ +// Copyright (C) 2019 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.googlesource.gerrit.plugins.healthcheck; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; +import com.google.gerrit.extensions.annotations.PluginName; +import com.google.gerrit.server.config.PluginConfigFactory; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.util.concurrent.TimeUnit; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; + +@Singleton +public class HealthCheckConfig { + public static final String HEALTHCHECK = "healthcheck"; + private final Config config; + private static final long HEALTHCHECK_TIMEOUT_DEFAULT = 500L; + public static final HealthCheckConfig DEFAULT_CONFIG = new HealthCheckConfig(null); + + @Inject + public HealthCheckConfig(PluginConfigFactory configFactory, @PluginName String pluginName) { + config = configFactory.getGlobalPluginConfig(pluginName); + } + + @VisibleForTesting + public HealthCheckConfig(String configText) { + config = new Config(); + if (!Strings.isNullOrEmpty(configText)) { + try { + config.fromText(configText); + } catch (ConfigInvalidException e) { + throw new IllegalArgumentException("Invalid configuration " + configText, e); + } + } + } + + public long getTimeout() { + return getTimeout(null); + } + + public long getTimeout(String healthCheckName) { + long defaultTimeout = healthCheckName == null ? HEALTHCHECK_TIMEOUT_DEFAULT : getTimeout(null); + return config.getTimeUnit( + HEALTHCHECK, healthCheckName, "timeout", defaultTimeout, TimeUnit.MILLISECONDS); + } +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java index a71e287..8aa6432 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
@@ -16,6 +16,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -24,16 +25,20 @@ public abstract class AbstractHealthCheck implements HealthCheck { private static final Logger log = LoggerFactory.getLogger(AbstractHealthCheck.class); - public static final long CHECK_TIMEOUT = 500L; + private final long timeout; private final String name; private final ListeningExecutorService executor; private final MetricsHandler metricsHandler; protected AbstractHealthCheck( - ListeningExecutorService executor, String name, MetricsHandler.Factory metricsHandler) { + ListeningExecutorService executor, + HealthCheckConfig config, + String name, + MetricsHandler.Factory metricsHandler) { this.executor = executor; this.name = name; this.metricsHandler = metricsHandler.create(name); + this.timeout = config.getTimeout(name); } @Override @@ -58,7 +63,7 @@ return new Status(healthy, ts, System.currentTimeMillis() - ts); }); try { - status = resultFuture.get(CHECK_TIMEOUT, TimeUnit.MILLISECONDS); + status = resultFuture.get(timeout, TimeUnit.MILLISECONDS); } catch (TimeoutException e) { log.warn("Check {} timed out", name, e); status = new Status(Result.TIMEOUT, ts, System.currentTimeMillis() - ts);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/JGitHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/JGitHealthCheck.java index 7de042c..90c9fd9 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/JGitHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/JGitHealthCheck.java
@@ -20,6 +20,7 @@ import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.inject.Inject; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; @@ -30,10 +31,11 @@ @Inject public JGitHealthCheck( ListeningExecutorService executor, + HealthCheckConfig config, GitRepositoryManager repositoryManager, AllProjectsName allProjectsName, MetricsHandler.Factory metricsHandlerFactory) { - super(executor, JGIT, metricsHandlerFactory); + super(executor, config, JGIT, metricsHandlerFactory); this.repositoryManager = repositoryManager; this.allProjectsName = allProjectsName; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ProjectsListHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ProjectsListHealthCheck.java index c31a286..0c9a120 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ProjectsListHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ProjectsListHealthCheck.java
@@ -20,6 +20,7 @@ import com.google.gerrit.extensions.common.ProjectInfo; import com.google.gerrit.server.project.ListProjects; import com.google.inject.Inject; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; import java.util.SortedMap; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -32,9 +33,10 @@ @Inject public ProjectsListHealthCheck( ListeningExecutorService executor, + HealthCheckConfig config, ListProjects listProjects, MetricsHandler.Factory metricHandlerFactory) { - super(executor, PROJECTSLIST, metricHandlerFactory); + super(executor, config, PROJECTSLIST, metricHandlerFactory); this.listProjects = listProjects; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ReviewDbHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ReviewDbHealthCheck.java index 7bd60cf..805eaa1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ReviewDbHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ReviewDbHealthCheck.java
@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; public class ReviewDbHealthCheck extends AbstractHealthCheck { private final SchemaFactory<ReviewDb> reviewDb; @@ -28,9 +29,10 @@ @Inject public ReviewDbHealthCheck( ListeningExecutorService executor, + HealthCheckConfig config, SchemaFactory<ReviewDb> reviewDb, MetricsHandler.Factory metricHandlerFactory) { - super(executor, REVIEWDB, metricHandlerFactory); + super(executor, config, REVIEWDB, metricHandlerFactory); this.reviewDb = reviewDb; }
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md new file mode 100644 index 0000000..b2f798d --- /dev/null +++ b/src/main/resources/Documentation/about.md
@@ -0,0 +1,3 @@ +This plugin provides support for monitoring and alerting purposes. +A common use-case is the automatic detection and recovery of issues +for Gerrit Servers that need to be available on a 24x7 basis.
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md new file mode 100644 index 0000000..5af0790 --- /dev/null +++ b/src/main/resources/Documentation/config.md
@@ -0,0 +1,56 @@ +@PLUGIN@ configuration +====================== + +The @PLUGIN@ +------------ + +The plugin does not require any configuration at all and exposes an HTTP +endpoint for querying the service status. + +``` +GET /config/server/healthcheck~status + +)]}' +{ + "ts": 139402910202, + "elapsed": 100, + "reviewdb": { + "ts": 139402910202, + "elapsed": 50, + "result": "passed" + }, + "projectslist": { + "ts": 139402910202, + "elapsed": 100, + "result": "passed" + }, + "auth": { + "ts": 139402910202, + "elapsed": 80, + "result": "passed" + } +} +``` + +Settings +-------- + +The plugin allows to customize its behaviour through a specific +`healthcheck.config` file in the `$GERRIT_SITE/etc` directory. + +Each section of the form `[healthcheck "<checkName>"]` can tailor the +behaviour of an individual `<checkName>`. The section `[healthcheck]` +defines the global defaults valid for all checks. + +The following check names are available: + +- `reviewdb` : check connectivity and ability to query ReviewDb +- `jgit` : check connectivity to the filesystem and ability to open a JGit ref and object +- `projectslist` : check the ability to list projects with their descriptions + +The follwing parameters are available: + +- `healthcheck.<checkName>.timeout` : Specific timeout (msec) for the + healthcheck to complete. Zero means that there is no timeout. + + Default: 500 \ No newline at end of file
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfigTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfigTest.java new file mode 100644 index 0000000..fc215ff --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfigTest.java
@@ -0,0 +1,48 @@ +// Copyright (C) 2019 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.googlesource.gerrit.plugins.healthcheck; + +import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig.DEFAULT_CONFIG; + +import org.junit.Test; + +public class HealthCheckConfigTest { + + @Test + public void shouldHaveDefaultTimeout() { + long defaultTimeout = DEFAULT_CONFIG.getTimeout(null); + assertThat(defaultTimeout).isGreaterThan(0L); + assertThat(DEFAULT_CONFIG.getTimeout("fooCheck")).isEqualTo(defaultTimeout); + } + + @Test + public void shouldHaveGlobalTimeout() { + HealthCheckConfig config = new HealthCheckConfig("[healthcheck]\n" + "timeout=1000"); + + assertThat(config.getTimeout(null)).isEqualTo(1000); + assertThat(config.getTimeout("barCheck")).isEqualTo(1000); + } + + @Test + public void shouldHaveCheckOverriddenTimeout() { + HealthCheckConfig config = + new HealthCheckConfig( + "[healthcheck]\n" + "timeout=2000\n" + "[healthcheck \"fooCheck\"]\n" + "timeout=1000"); + + assertThat(config.getTimeout("fooCheck")).isEqualTo(1000); + assertThat(config.getTimeout("barCheck")).isEqualTo(2000); + } +}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckStatusEndpointTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckStatusEndpointTest.java index 1d46933..4bc63ba 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckStatusEndpointTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckStatusEndpointTest.java
@@ -15,7 +15,9 @@ package com.googlesource.gerrit.plugins.healthcheck; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig.DEFAULT_CONFIG; +import com.google.common.util.concurrent.MoreExecutors; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.metrics.DisabledMetricMaker; @@ -24,30 +26,44 @@ import com.google.inject.Guice; import com.google.inject.Injector; import com.googlesource.gerrit.plugins.healthcheck.api.HealthCheckStatusEndpoint; +import com.googlesource.gerrit.plugins.healthcheck.check.AbstractHealthCheck; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; +import com.googlesource.gerrit.plugins.healthcheck.check.MetricsHandler; +import java.util.concurrent.Executors; import javax.servlet.http.HttpServletResponse; import org.junit.Test; public class HealthCheckStatusEndpointTest { - public static class TestHealthCheck implements HealthCheck { - private final HealthCheck.Status checkResult; - private final String checkName; + public static class TestHealthCheck extends AbstractHealthCheck { + private final HealthCheck.Result checkResult; + private final long sleep; - public TestHealthCheck(String checkName, HealthCheck.Result result, long ts, long elapsed) { - this.checkName = checkName; - this.checkResult = new Status(result, ts, elapsed); + public TestHealthCheck( + HealthCheckConfig config, String checkName, HealthCheck.Result result, long sleep) { + super( + MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(10)), + config, + checkName, + new MetricsHandler.Factory() { + + @Override + public MetricsHandler create(String name) { + return new MetricsHandler(checkName, new DisabledMetricMaker()); + } + }); + this.checkResult = result; + this.sleep = sleep; } @Override - public Status run() { + public Result doCheck() { + try { + Thread.sleep(sleep); + } catch (InterruptedException e) { + } return checkResult; } - - @Override - public String name() { - return checkName; - } } @Test @@ -58,7 +74,10 @@ @Override protected void configure() { DynamicSet.bind(binder(), HealthCheck.class) - .toInstance(new TestHealthCheck("checkOk", HealthCheck.Result.PASSED, 1, 2)); + .toInstance( + new TestHealthCheck( + DEFAULT_CONFIG, "checkOk", HealthCheck.Result.PASSED, 0)); + bind(HealthCheckConfig.class).toInstance(DEFAULT_CONFIG); DynamicSet.bind(binder(), MetricMaker.class).toInstance(new DisabledMetricMaker()); } }); @@ -71,6 +90,29 @@ } @Test + public void shouldReturnServerErrorWhenOneChecksTimesOut() throws Exception { + Injector injector = + testInjector( + new AbstractModule() { + @Override + protected void configure() { + HealthCheckConfig config = + new HealthCheckConfig("[healthcheck]\n" + "timeout = 20"); + DynamicSet.bind(binder(), HealthCheck.class) + .toInstance( + new TestHealthCheck(config, "checkOk", HealthCheck.Result.PASSED, 30)); + bind(HealthCheckConfig.class).toInstance(config); + } + }); + + HealthCheckStatusEndpoint healthCheckApi = + injector.getInstance(HealthCheckStatusEndpoint.class); + Response<?> resp = (Response<?>) healthCheckApi.apply(null); + + assertThat(resp.statusCode()).isEqualTo(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } + + @Test public void shouldReturnServerErrorWhenAtLeastOneCheckIsFailing() throws Exception { Injector injector = testInjector( @@ -78,9 +120,13 @@ @Override protected void configure() { DynamicSet.bind(binder(), HealthCheck.class) - .toInstance(new TestHealthCheck("checkOk", HealthCheck.Result.PASSED, 1, 2)); + .toInstance( + new TestHealthCheck( + DEFAULT_CONFIG, "checkOk", HealthCheck.Result.PASSED, 0)); DynamicSet.bind(binder(), HealthCheck.class) - .toInstance(new TestHealthCheck("checkKo", HealthCheck.Result.FAILED, 1, 2)); + .toInstance( + new TestHealthCheck( + DEFAULT_CONFIG, "checkKo", HealthCheck.Result.FAILED, 0)); } });
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/JGitHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/JGitHealthCheckTest.java index b9beb77..6e78dfe 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/JGitHealthCheckTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/JGitHealthCheckTest.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.healthcheck; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig.DEFAULT_CONFIG; import static org.eclipse.jgit.lib.RefUpdate.Result.NEW; import com.google.common.util.concurrent.ListeningExecutorService; @@ -70,7 +71,11 @@ public void shouldBeHealthyWhenJGitIsWorking() { JGitHealthCheck reviewDbCheck = new JGitHealthCheck( - executor, getWorkingRepositoryManager(), allProjectsName, metricsHandlerFactory); + executor, + DEFAULT_CONFIG, + getWorkingRepositoryManager(), + allProjectsName, + metricsHandlerFactory); assertThat(reviewDbCheck.run().result).isEqualTo(Result.PASSED); } @@ -78,7 +83,11 @@ public void shouldBeUnhealthyWhenJGitIsFailing() { JGitHealthCheck jGitHealthCheck = new JGitHealthCheck( - executor, getFailingGitRepositoryManager(), allProjectsName, metricsHandlerFactory); + executor, + DEFAULT_CONFIG, + getFailingGitRepositoryManager(), + allProjectsName, + metricsHandlerFactory); assertThat(jGitHealthCheck.run().result).isEqualTo(Result.FAILED); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java index 942af0c..e07c12c 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.healthcheck; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig.DEFAULT_CONFIG; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.extensions.common.ProjectInfo; @@ -23,7 +24,6 @@ import com.google.gerrit.server.project.ListProjects; import com.google.inject.Guice; import com.google.inject.Inject; -import com.googlesource.gerrit.plugins.healthcheck.check.AbstractHealthCheck; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result; import com.googlesource.gerrit.plugins.healthcheck.check.MetricsHandler; import com.googlesource.gerrit.plugins.healthcheck.check.ProjectsListHealthCheck; @@ -43,21 +43,23 @@ @Inject private ListeningExecutorService executor; @Before - public void setUp() { + public void setUp() throws Exception { Guice.createInjector(new HealthCheckModule()).injectMembers(this); } @Test public void shouldBeHealthyWhenListProjectsWorks() { ProjectsListHealthCheck jGitHealthCheck = - new ProjectsListHealthCheck(executor, getWorkingProjectList(0), metricsHandlerFactory); + new ProjectsListHealthCheck( + executor, DEFAULT_CONFIG, getWorkingProjectList(0), metricsHandlerFactory); assertThat(jGitHealthCheck.run().result).isEqualTo(Result.PASSED); } @Test public void shouldBeUnhealthyWhenListProjectsIsFailing() { ProjectsListHealthCheck jGitHealthCheck = - new ProjectsListHealthCheck(executor, getFailingProjectList(), metricsHandlerFactory); + new ProjectsListHealthCheck( + executor, DEFAULT_CONFIG, getFailingProjectList(), metricsHandlerFactory); assertThat(jGitHealthCheck.run().result).isEqualTo(Result.FAILED); } @@ -66,7 +68,8 @@ ProjectsListHealthCheck jGitHealthCheck = new ProjectsListHealthCheck( executor, - getWorkingProjectList(AbstractHealthCheck.CHECK_TIMEOUT * 2), + DEFAULT_CONFIG, + getWorkingProjectList(DEFAULT_CONFIG.getTimeout() * 2), metricsHandlerFactory); assertThat(jGitHealthCheck.run().result).isEqualTo(Result.TIMEOUT); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ReviewDbHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ReviewDbHealthCheckTest.java index 6c5e008..fc818b7 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ReviewDbHealthCheckTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ReviewDbHealthCheckTest.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.healthcheck; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig.DEFAULT_CONFIG; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.lifecycle.LifecycleManager; @@ -54,14 +55,16 @@ @Test public void shouldBeHealthyWhenReviewDbIsWorking() { ReviewDbHealthCheck reviewDbCheck = - new ReviewDbHealthCheck(executor, workingReviewDbFactory, metricsHandlerFactory); + new ReviewDbHealthCheck( + executor, DEFAULT_CONFIG, workingReviewDbFactory, metricsHandlerFactory); assertThat(reviewDbCheck.run().result).isEqualTo(HealthCheck.Result.PASSED); } @Test public void shouldBeUnhealthyWhenReviewDbIsFailing() { ReviewDbHealthCheck reviewDbCheck = - new ReviewDbHealthCheck(executor, getFailingReviewDbProvider(), metricsHandlerFactory); + new ReviewDbHealthCheck( + executor, DEFAULT_CONFIG, getFailingReviewDbProvider(), metricsHandlerFactory); assertThat(reviewDbCheck.run().result).isEqualTo(HealthCheck.Result.FAILED); }