feat: add health check endpoint #3501

vcrfxia · 2019-10-08T23:46:47Z

Description

Adds a new HTTP endpoint (/healthcheck) to return high-level information about server health. The endpoint currently performs two checks, a metastore check (list streams; list tables; list queries;) and a Kafka connectivity check (list topics extended;). The results are structured as follows:

$ curl -X GET http://localhost:8088/healthcheck | jq
{
  "isHealthy": true,
  "details": {
    "metastore": {
      "isHealthy": true
    },
    "kafka": {
      "isHealthy": true
    }
  }
}

Because the endpoint is unauthenticated, requests from the healthcheck endpoint to the KSQL server are rate-limited according to the new config ksql.healthcheck.interval.ms: healthcheck results are cached and only re-issued if more than ksql.healthcheck.interval.ms have passed since the healthcheck last issued requests to the KSQL server.

This PR looks big but is actually quite simple:

HealthcheckResource specifies the new endpoint.
HealthcheckAgent specifies the checks that are run.
HealthcheckResponse and HealthcheckResponseDetail specify the structure of the response.
ServerInternalKsqlClient is a KSQL client that sends requests directly to KsqlResource, rather than going through the usual network and authentication paths. An earlier iteration of this PR used the regular KSQL client in the ServiceContext instead, but this approach fails without a way to pass required authentication credentials to the client.
Some methods were moved from KsqlTarget to a new util file KsqlClientUtil so they are accessible to ServerInternalKsqlClient.

Testing done

Unit tests, integration test, and manual.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

stevenpyzhang

Thanks @vcrfxia , LGTM overall with some questions.

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/HealthcheckResource.java

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/HealthcheckResponseDetail.java

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

ksql-rest-app/src/test/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgentTest.java

stevenpyzhang · 2019-10-09T17:32:11Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

+public class HealthcheckAgent {
+
+  private static final List<Check> DEFAULT_CHECKS = ImmutableList.of(
+      new Check("metastore", "list streams; list tables; list queries;"),


I think it would be better to make all these public static String variables from the HealthCheckAgent class. That way the constants can also be used in the test for this class and there won't be hardcoded strings split between this class and test.

It feels odd to me to have these statements in non-private variables, since no other class (besides the test class) needs to access them. If the issue is the hard-coded strings, I can pull them out into private (static) variables, but they'd be duplicated in the test anyway.

I was thinking if we were going to make a change to which statements to execute for the health check, having it not be hardcoded in the two files would make things easier to change. It's only a minor optimization though so if you don't think it's worth it then I'm fine with how it is now.

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/HealthcheckResponse.java

rodesai

Nice! Left a couple bits of feedback inline.

rodesai · 2019-10-09T21:17:34Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

+
+public class HealthcheckAgent {
+
+  private static final List<Check> DEFAULT_CHECKS = ImmutableList.of(


This can be a bit more abstract. You just need an interface like:

interface Check { String name(); HealthcheckResponseDetail check(); }

The current Check class can just be an implementation of this (e.g. ExecuteStatementCheck). This way we can support more diverse extensions down the road.

Done, though I updated the signature of the check() method to take a KSQL client and server endpoint, to avoid needing to pass those into each of the individual checks.

rodesai · 2019-10-10T00:02:20Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/HealthcheckResource.java

+  }
+
+  private HealthcheckResponse getResponse() {
+    if (responseCache.isEmpty() || timeSinceLastResponse().compareTo(healthcheckInterval) > 0) {


it would be cleaner to move the expiry into the cache, e.g.:

final Optional<HealthCheckResponse> response = responseCache.get(); if (response.isPresent()) { return response.get(); } final HealthcheckResponse fresh = healtcheckAgent.checkHealth(); responseCache.cache(fresh); return fresh;

and in the Cache, get() can return empty if nothing is cached or if its expired.

Done, though I wonder if it's clear from just reading

final Optional<HealthCheckResponse> response = responseCache.get(); if (response.isPresent()) { return response.get(); }

that the cache handles expiration (rather than returning any previously cached response). Maybe I should update the cache name to be ExpiringResponseCache or something similar?

Maybe if we change the get() method to getIfPresent() it'll be clearer?

I think the fact that the return type is an Optional already indicates that "if present" part. It was the fact that the cache expires that I didn't think was captured. I don't want to block the PR on this though, can always open a follow-up to change the name.

getIfUnexpired() then to indicate expiration behavior? I do agree this is a minor point.

vcrfxia

@rodesai pointed out that using a real KsqlClient as I have in my first implementation means healthcheck requests will fail unless relevant authentication credentials are provided for the client. As such, I’ve updated the HealthcheckAgent to use an internal KsqlClient (to send requests to KsqlResource directly).

vcrfxia · 2019-10-10T00:03:39Z

ksql-rest-client/src/main/java/io/confluent/ksql/rest/client/KsqlClientUtil.java

+  private KsqlClientUtil() {
+  }
+
+  public static <T> RestResponse<T> toRestResponse(


The contents of this new util file were lifted from KsqlTarget, so the methods may be accessible to the new ServerInternalKsqlClient as well.

vcrfxia · 2019-10-10T00:05:14Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

+    return response.isSuccessful();
+  }
+
+  private static URI getServerAddress(final KsqlRestConfig restConfig) {


This isn't actually needed anymore since the ServerInternalKsqlClient used by the HealthcheckAgent ignores the serverEndpoint parameter in makeKsqlRequest() but I've left this code here in case we'd like to switch back to using a real KSQL client in the future. If preferable I can simply delete it for now instead.

...-rest-app/src/main/java/io/confluent/ksql/rest/server/services/ServerInternalKsqlClient.java

vcrfxia

Thanks for the reviews, @stevenpyzhang and @rodesai ! Applied your feedback, and also added a short section into the docs.

vcrfxia · 2019-10-10T20:59:49Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

+
+public class HealthcheckAgent {
+
+  private static final List<Check> DEFAULT_CHECKS = ImmutableList.of(


Done, though I updated the signature of the check() method to take a KSQL client and server endpoint, to avoid needing to pass those into each of the individual checks.

vcrfxia · 2019-10-10T21:02:09Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

+public class HealthcheckAgent {
+
+  private static final List<Check> DEFAULT_CHECKS = ImmutableList.of(
+      new Check("metastore", "list streams; list tables; list queries;"),


It feels odd to me to have these statements in non-private variables, since no other class (besides the test class) needs to access them. If the issue is the hard-coded strings, I can pull them out into private (static) variables, but they'd be duplicated in the test anyway.

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

vcrfxia · 2019-10-10T21:03:41Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/HealthcheckResource.java

+  }
+
+  private HealthcheckResponse getResponse() {
+    if (responseCache.isEmpty() || timeSinceLastResponse().compareTo(healthcheckInterval) > 0) {


Done, though I wonder if it's clear from just reading

final Optional<HealthCheckResponse> response = responseCache.get(); if (response.isPresent()) { return response.get(); }

that the cache handles expiration (rather than returning any previously cached response). Maybe I should update the cache name to be ExpiringResponseCache or something similar?

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthcheckAgent.java

JimGalasyn · 2019-10-10T21:14:24Z

docs/developer-guide/api.rst

+
+Your output should resemble:
+
+.. codewithvars:: bash


Suggested change

.. codewithvars:: bash

.. codewithvars:: json

JimGalasyn

LGTM, with one tiny suggestion!

spena · 2019-10-11T19:32:03Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/HealthCheckResource.java

+  }
+
+  /* Caches a HealthCheckResponse for the specified duration */
+  private static class ResponseCache {


You can make a great and simple cache (in a few lines) by using the Guava cache library.
See https://www.baeldung.com/guava-cache (search for evict records based on their total live time)

You could use a boolean key on the cache where to store the healthcheck response.

Done, though the downside is I can no longer use a custom time supplier in the unit tests. One of the updated tests currently sleeps for 11 ms, but hopefully that's OK?

(Also, thanks to @agavra for pointing out that switching to Guava's implementation means we get concurrency handling for free!)

Great!. Yes, guava will help you with concurrency as well. Regarding the sleep time, can the test use a 0ms interval health check, then sleep for 1ms in the test so you check the past health check was evicted?

Btw, I think you can even reduce more code by using the CacheLoader and not the ResponseCache. Here's a code snippet I just imagined how this would look like:

private static final boolean HEALTHCHECK = true; private static final CacheLoader<Boolean, HealthCheckResponse> HEALTHCHECK_LOADER = new CacheLoader<Boolean, HealthCheckResponse>() { @Override public HealthCheckResponse load(Boolean a) throws Exception { return healthCheckAgent.checkHealth(); } }; private final LoadingCache<Boolean, HealthCheckResponse> cache; HealthCheckResource(final Duration healthCheckInterval) { cache = CacheBuilder.newBuilder() .expireAfterWrite(healthCheckInterval.toMillis(), TimeUnit.MILLISECONDS) .build(HEALTHCHECK_LOADER); } @GET public Response checkHealth() { return Response.ok(getResponse()).build(); } private HealthCheckResponse getResponse() { // This will call the healthCheckAgent.checkHealth() when the interval time has passed return cache.getUnchecked(HEALTHCHECK); }

Regarding the sleep time, can the test use a 0ms interval health check, then sleep for 1ms in the test so you check the past health check was evicted?

See discussion here: #3501 (comment)

Btw, I think you can even reduce more code by using the CacheLoader and not the ResponseCache.

Sure, I can update to use a LoadingCache instead. My reason for wrapping the Guava cache in an internal ResponseCache class was to ensure that only a single key is ever used with the cache, but I can move this enforcement into the CacheLoader so I'll do that instead.

spena · 2019-10-11T19:39:17Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthCheckAgent.java

+    final boolean allHealthy = results.values().stream()
+        .map(HealthCheckResponseDetail::getIsHealthy)
+        .reduce(Boolean::logicalAnd)
+        .orElse(true);


You can reduce this to:

results.values.stream() .allMatch(HealthCheckResponseDetail::getIsHealthy);

spena · 2019-10-11T19:48:11Z

...-rest-app/src/main/java/io/confluent/ksql/rest/server/services/ServerInternalKsqlClient.java

+
+  @Override
+  public RestResponse<KsqlEntityList> makeKsqlRequest(
+      final URI serverEndpoint,


This serverEndpoint is unused, right?

Yup: #3501 (comment)

spena · 2019-10-11T19:53:06Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthCheckAgent.java

+        final URI serverEndpoint
+    ) {
+      final RestResponse<KsqlEntityList> response =
+          ksqlClient.makeKsqlRequest(serverEndpoint, ksqlStatement);


I feel that the 3 lines from SimpleKsqlClient can be moved here instead, doesn't it?

The serverEndPoint is unused there, and the sql is already in the ksqlStatement. How could I use the SimpleKsqlClient in other code besides the health check?

Other SimpleKsqlClient implementations exist, including https://github.com/confluentinc/ksql/blob/c0bfa41a109580e76c76cf4fa23b603646ac40f2/ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/services/DefaultKsqlClient.java and https://github.com/confluentinc/ksql/blob/c0bfa41a109580e76c76cf4fa23b603646ac40f2/ksql-execution/src/main/java/io/confluent/ksql/services/DisabledKsqlClient.java, and is used by the StaticQueryExecutor:

ksql/ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/StaticQueryExecutor.java

Line 666 in c0bfa41

final RestResponse<KsqlEntityList> response = serviceContext

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/HealthCheckResponse.java

vcrfxia

Thanks for the review, @spena ! Applied your suggestions.

vcrfxia · 2019-10-14T20:33:03Z

...-rest-app/src/main/java/io/confluent/ksql/rest/server/services/ServerInternalKsqlClient.java

+
+  @Override
+  public RestResponse<KsqlEntityList> makeKsqlRequest(
+      final URI serverEndpoint,


Yup: #3501 (comment)

vcrfxia · 2019-10-14T20:33:51Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/HealthCheckResource.java

+  }
+
+  /* Caches a HealthCheckResponse for the specified duration */
+  private static class ResponseCache {


Done, though the downside is I can no longer use a custom time supplier in the unit tests. One of the updated tests currently sleeps for 11 ms, but hopefully that's OK?

(Also, thanks to @agavra for pointing out that switching to Guava's implementation means we get concurrency handling for free!)

vcrfxia · 2019-10-14T20:36:55Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/healthcheck/HealthCheckAgent.java

+        final URI serverEndpoint
+    ) {
+      final RestResponse<KsqlEntityList> response =
+          ksqlClient.makeKsqlRequest(serverEndpoint, ksqlStatement);


Other SimpleKsqlClient implementations exist, including https://github.com/confluentinc/ksql/blob/c0bfa41a109580e76c76cf4fa23b603646ac40f2/ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/services/DefaultKsqlClient.java and https://github.com/confluentinc/ksql/blob/c0bfa41a109580e76c76cf4fa23b603646ac40f2/ksql-execution/src/main/java/io/confluent/ksql/services/DisabledKsqlClient.java, and is used by the StaticQueryExecutor:

ksql/ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/StaticQueryExecutor.java

Line 666 in c0bfa41

final RestResponse<KsqlEntityList> response = serviceContext

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/HealthCheckResponse.java

vinothchandar · 2019-10-14T23:57:53Z

...-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/HealthCheckResourceTest.java

    healthCheckResource.checkHealth();
+    Thread.sleep(11);


1 ms extra wait is too tight? could potentially be flaky? is there a better way to check this by waiting until a condition is met? e.g https://github.com/awaitility/awaitility

In this case, we would keep fetching the old value out of the cache in a loop and exit out when the underlying method to recheck health has been called once?

Thanks for the tip! Updated the test.

spena

Looks good!

vcrfxia added 5 commits October 8, 2019 01:00

feat: add healthcheck endpoint

fd22534

feat: switch to configexception

6d10311

feat: make healthcheck interval configurable

5abf820

feat: update healthcheck response detail to be a class

4cebe97

feat: healthcheck should not be authenticated

34441bd

vcrfxia requested a review from a team as a code owner October 8, 2019 23:46

stevenpyzhang approved these changes Oct 9, 2019

View reviewed changes

vcrfxia added 3 commits October 9, 2019 13:08

Merge branch 'master' into healthcheck-endpoint

89297e6

refactor: split out KsqlTarget helper methods

47818cf

fix: use ServerInternalKsqlClient to get around auth issues

d619f74

rodesai reviewed Oct 10, 2019

View reviewed changes

vcrfxia commented Oct 10, 2019

View reviewed changes

stevenpyzhang reviewed Oct 10, 2019

View reviewed changes

...-rest-app/src/main/java/io/confluent/ksql/rest/server/services/ServerInternalKsqlClient.java Show resolved Hide resolved

vcrfxia added 5 commits October 10, 2019 11:56

fix: make Check an interface

612f0c0

fix: move cached response expiry into ResponseCache

f795d34

docs: add java doc to ServerInternalKsqlClient

5fa0002

refactor: make health check two words

e8e0647

docs: add docs for new endpoint

fc0bbba

vcrfxia commented Oct 10, 2019

View reviewed changes

vcrfxia requested a review from JimGalasyn as a code owner October 10, 2019 21:05

vcrfxia changed the title ~~feat: add healthcheck endpoint~~ feat: add health check endpoint Oct 10, 2019

JimGalasyn reviewed Oct 10, 2019

View reviewed changes

JimGalasyn approved these changes Oct 10, 2019

View reviewed changes

docs: jim's feedback

cae31e6

spena reviewed Oct 11, 2019

View reviewed changes

vcrfxia added 2 commits October 14, 2019 13:18

refactor: switch to Guava's cache implementation

548ca3c

fix: sergio's feedback

3a754bd

vcrfxia commented Oct 14, 2019

View reviewed changes

fix: checkstyle

ef0797f

vinothchandar reviewed Oct 14, 2019

View reviewed changes

vcrfxia added 3 commits October 14, 2019 17:00

fix: fix bug in getting entity from response. add integration test

2611061

test: vinoth's feedback

6d2103b

refactor: switch to LoadingCache

a678d9e

vcrfxia requested a review from spena October 15, 2019 20:55

spena approved these changes Oct 15, 2019

View reviewed changes

vcrfxia added 3 commits October 15, 2019 15:09

style: checkstyle

01cf409

fix: findbugs

8b176f3

fix: make check names public so clients don't have to hardcode

0db13de

vcrfxia merged commit 2308686 into confluentinc:master Oct 16, 2019

vcrfxia deleted the healthcheck-endpoint branch October 16, 2019 03:34


		public class HealthcheckAgent {

		private static final List<Check> DEFAULT_CHECKS = ImmutableList.of(

feat: add health check endpoint #3501

feat: add health check endpoint #3501

Conversation

vcrfxia commented Oct 8, 2019 • edited Loading

Description

Testing done

Reviewer checklist

stevenpyzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenpyzhang Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimGalasyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spena Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spena left a comment

Choose a reason for hiding this comment

vcrfxia commented Oct 8, 2019 •

edited

Loading

stevenpyzhang Oct 11, 2019 •

edited

Loading

spena Oct 11, 2019 •

edited

Loading