Health Check Api #159

lamdav · 2018-03-18T23:31:43Z

https://docs.gitlab.com/ce/user/admin_area/monitoring/health_check.html

This is a mostly admin endpoint.

lamdav · 2018-03-18T23:36:31Z

src/main/java/org/gitlab4j/api/GitLabApiClient.java

@@ -46,6 +46,7 @@

    private ClientConfig clientConfig;
    private Client apiClient;
+    private String baseUrl;


Added a field to keep track of the original url.

lamdav · 2018-03-18T23:37:35Z

src/main/java/org/gitlab4j/api/GitLabApiClient.java

            }
        }
-
-        return (new URL(url.toString()));
+        return urlBuilder.toString();


factored out url building logic so that it can be applied with the baseUrl as well.

lamdav · 2018-03-18T23:39:00Z

src/main/java/org/gitlab4j/api/HealthCheckApi.java

+     * @param token Health Status token
+     * @return LivenessHealthCheck instance
+     * @throws GitLabApiException if any exception occurs
+     * @deprecated


Gitlab docs state these have been deprecated. I added them in any way so that it is at least backward compatible during the deprecation process.

lamdav · 2018-03-18T23:39:46Z

src/main/java/org/gitlab4j/api/models/HealthCheckStatus.java

+
+@XmlAccessorType(XmlAccessType.FIELD)
+public class HealthCheckStatus {
+    private String status;


Ideally, these should be enums but I don't know all the possible values other than "ok". If you have any ideas, I would be happy to update this.

Check out the source for the health controller at:
https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/health_controller.rb

Valid status are: "ok", "failed"

lamdav · 2018-03-18T23:41:37Z

src/main/java/org/gitlab4j/api/models/HealthCheckStatusLabel.java

+
+@XmlAccessorType(XmlAccessType.FIELD)
+public class HealthCheckStatusLabel {
+    private String shard;


The only extra label I had was "shard". If you have an idea of other labels, I am willing to add them.

Ideally, this should be an enum as well but I have not encountered other values while testing.

Looking at the source for the health check controller, I do not believe that the labels should be enums, they instead should be a dictionary of name/value pairs.

Making them a dictionary would also eliminate the need for this class.

lamdav

I left some comments pointing out a few things. Let me know what you think.

gmessner · 2018-03-19T04:03:34Z

@lamdav
I have avoided adding functionality that is not part of the GitLab API, this health check API is part of the admin dashboard and requires an entirely different access token than the API does. This could possibly lead to confusion when the same access token doesn't work with the other API calls.

I guess what I'm saying is that I'm not sure this belongs here, though you might be able to convince me otherwise.

lamdav · 2018-03-19T04:56:49Z

@gmessner
Thanks for taking a look.

If I remove the deprecated call, there would be no methods that take a new token. That is, there should be no confusion between api token and monitoring token as the preferred method of accessing this gitlab endpoint is through ip_whitelisting. This is further added in the javadoc upon 404 errors if the user is confused.

As for your question on whether this belongs here or not, I do not think I have a convincing argument. I will elaborate on my desired usage. I am working on a project that integrates heavily with gitlab and several other components. I would like to create a monitoring dashboard that monitors all components. Since I am already using a gitlab wrapper, I would rather use the wrapper's abstractions for gitlab to query their health endpoints. This will help keep all application communication through gitlab localize through the library. The alternative, and in my opinion, less elegant solution is to use barebone java to do so and parse the results. Other alternatives would include adding dependencies which I think is overkill for one kind of query.

If your main concern is possible confusion, then perhaps we can work with the GitlabApi class and have a method called getAdminApi() in which it constructs a class GitLabAdminApi which wraps things such as HealthCheckApi and other admin level apis. This way the intent is clear to the user that using this api requires elevated permissions.

Here is what it would look:

gitlabApi.getAdminApi().getHealthCheckApi().getLiveness();

Let me know what you think.

gmessner · 2018-03-20T19:10:47Z

I think you are on the right track with making the AdminApi separate, however I think that we should not make the AdminApi/HealthCheckApi part of the GitLabApi as it will require one to properly instantiate a GitLabApi instance to get to the AdminApi HealthCheckApi instance. I have an idea on how to make it separate, when I get home tonight I will provide more info on that or maybe even create it.

lamdav · 2018-04-19T05:46:51Z

@gmessner Can I get a status update on this?

gmessner

After looking at the source for the health_controller at:
https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/health_controller.rb

I believe both readiness and liveliness should use the same class for results, so I would like to see just a single results class called: HealthCheckInfo

HealthCheckInfo should be comprised of HealthCheckItem (was HealthCheckStatus) which includes the status, labels (now a Map<String, String>), and message properties.

gmessner · 2018-04-19T23:40:12Z

src/main/java/org/gitlab4j/api/HealthCheckApi.java

+    /**
+     * Get Health Checks from the liveness endpoint.
+     * Requires ip_whitelist
+     *


Clarify "Requires ip_whitelist". This should provide clear instructions for the IP whitelist, you could provide the URL:
https://docs.gitlab.com/ee/administration/monitoring/ip_whitelist.html

Good idea. I put the url for all methods doc strings.

gmessner · 2018-04-19T23:53:23Z

src/main/java/org/gitlab4j/api/models/HealthCheckStatusLabel.java

+
+@XmlAccessorType(XmlAccessType.FIELD)
+public class HealthCheckStatusLabel {
+    private String shard;


Looking at the source for the health check controller, I do not believe that the labels should be enums, they instead should be a dictionary of name/value pairs.

Making them a dictionary would also eliminate the need for this class.

gmessner · 2018-04-19T23:58:53Z

src/main/java/org/gitlab4j/api/models/HealthCheckStatus.java

+
+@XmlAccessorType(XmlAccessType.FIELD)
+public class HealthCheckStatus {
+    private String status;


Check out the source for the health controller at:
https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/health_controller.rb

Valid status are: "ok", "failed"

gmessner · 2018-04-20T00:16:07Z

src/main/java/org/gitlab4j/api/models/HealthCheckStatusDetail.java

+import javax.xml.bind.annotation.XmlAccessorType;
+
+@XmlAccessorType(XmlAccessType.FIELD)
+public class HealthCheckStatusDetail extends HealthCheckStatus {


No longer needed after refactoring LivenessHealthCheck.java to HealthCheckInfo.java

gmessner · 2018-04-20T00:17:53Z

src/main/java/org/gitlab4j/api/models/HealthCheckStatus.java

+import javax.xml.bind.annotation.XmlAccessorType;
+
+@XmlAccessorType(XmlAccessType.FIELD)
+public class HealthCheckStatus {


Rename this HealthCheckItem and include the following properties

private HealthCheckStatus status; private Map<String, String> labels; private String message;

gmessner · 2018-04-20T00:19:12Z

src/main/java/org/gitlab4j/api/models/LivenessHealthCheck.java

+
+@XmlRootElement
+@XmlAccessorType(XmlAccessType.FIELD)
+public class LivenessHealthCheck {


Rename to HealthCheckInfo and use the newly refactored HealthCheckItem instead of HealthCheckStatus

gmessner · 2018-04-20T00:19:52Z

src/main/java/org/gitlab4j/api/models/ReadinessHealthCheck.java

+
+@XmlRootElement
+@XmlAccessorType(XmlAccessType.FIELD)
+public class ReadinessHealthCheck {


No longer needed, use HealthCheckInfo as the class for both readiness and liveliness

gmessner · 2018-04-20T00:24:50Z

I have decided to go with your changes to GitLabApi and GitLabApiClient, however after reviewing the health_controller.rb source I believe your implementation can be greatly simplified. I have started a code review.

You'll find the source for the health_controller at: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/health_controller.rb

lamdav · 2018-04-20T05:46:40Z

@gmessner Thanks for the feedback. I think I made the changes you suggested accordingly. Let me know if I misunderstood something or if you have any other changes you would like to make.

gmessner · 2018-04-20T05:51:20Z

README.md

+```java
+// Get the liveness endpoint health check results.
+// Assumes ip_whitelisted
+LivenessHealthCheck healthCheck = gitLabApi.getHealthCheckApi().getLiveness();


LivenessHealthCheck should be HealthCheckInfo.

Nevermind, since I will be editing this file to bump the version when I release, I can make the edit then.

gmessner · 2018-04-20T05:53:37Z

@lamdav
Changes are spot on. Thanks for your contribution and my apologies for taking so long to get to it

gmessner · 2018-04-20T07:08:59Z

@lamdav
I added unit tests, made a few minor changes, and released 4.8.12. Thanks again for your contribution.

lamdav commented Mar 18, 2018

View reviewed changes

gmessner requested changes Apr 20, 2018

View reviewed changes

lamdav added 2 commits April 20, 2018 01:22

add Health Status Api support

2e15348

simplified HealthCheck classes

743d1a7

lamdav force-pushed the feature/health-check-api branch from f62ab0d to 743d1a7 Compare April 20, 2018 05:44

gmessner reviewed Apr 20, 2018

View reviewed changes

gmessner merged commit 900c1a9 into gitlab4j:master Apr 20, 2018

gmessner added a commit that referenced this pull request Apr 20, 2018

Added unit tests for HealthCheckApi (#159).

9d4e117

gmessner added a commit that referenced this pull request Apr 20, 2018

Added unit tests for HealthCheckApi (#159).

5fa1cc2

gmessner added a commit that referenced this pull request Apr 20, 2018

Added unit tests for HealthCheckApi (#159).

a519716

gmessner added a commit that referenced this pull request Apr 20, 2018

Added unit tests for HealthCheckApi (#159).

f1d3f7e

gmessner added a commit that referenced this pull request Apr 20, 2018

Added unit tests for HealthCheckApi (#159).

220702a

gmessner added a commit that referenced this pull request Apr 20, 2018

Added unit tests for HealthCheckApi (#159).

9f75e80

gmessner added the enhancement label Apr 20, 2018

Health Check Api #159

Health Check Api #159

Uh oh!

Conversation

lamdav commented Mar 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lamdav left a comment

Choose a reason for hiding this comment

Uh oh!

gmessner commented Mar 19, 2018

Uh oh!

lamdav commented Mar 19, 2018

Uh oh!

gmessner commented Mar 20, 2018

Uh oh!

lamdav commented Apr 19, 2018

Uh oh!

gmessner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmessner Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmessner Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmessner Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmessner Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmessner commented Apr 20, 2018

Uh oh!

lamdav commented Apr 20, 2018

Uh oh!

gmessner Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmessner commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmessner commented Apr 20, 2018

Uh oh!

Uh oh!

gmessner left a comment •

edited

Loading

gmessner Apr 20, 2018 •

edited

Loading

gmessner Apr 20, 2018 •

edited

Loading

gmessner Apr 20, 2018 •

edited

Loading

gmessner Apr 20, 2018 •

edited

Loading

gmessner Apr 20, 2018 •

edited

Loading

gmessner commented Apr 20, 2018 •

edited

Loading