Skip to content

Add Metrics for External Custom Health Check Requests#1383

Merged
huizhilu merged 3 commits intoapache:masterfrom
huizhilu:custom-metrics
Sep 30, 2020
Merged

Add Metrics for External Custom Health Check Requests#1383
huizhilu merged 3 commits intoapache:masterfrom
huizhilu:custom-metrics

Conversation

@huizhilu
Copy link
Contributor

@huizhilu huizhilu commented Sep 20, 2020

Issues

  • My PR addresses the following Helix issues and references them in the PR description:

Resolves #1382

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Custom rest client interacts with participant side to query for its health checks. Currently there is no metrics for the query requests. We'd like to have metrics for the requests.

Here's a design trade-off:

  1. Add a new API getInstanceStoppableCheck(baseUrl, customPayloads, namespace) in interface CustomRestClient. We need the namespace name when recording metrics in CustomRestClient. Since CustomRestClient is a singleton instance, we can not inject the namespace to the class. Having the namespace as a parameter in the API is an option. The benefit is we can accurately record the http request latency, and it is more scalable if we have more places calling the API, we don't need to do anything. Cons: change existing interface to add the new API. Though it is an interface, we may not expect external users to use it. So it should be fine to change the interface.
  2. Record the metrics in the upstream caller outside CustomRestClient, eg. InstanceService where getInstanceStoppableCheck() is called, we record the metrics. Pros: we don't need to change existing interface CustomRestClient. Cons: Latency of http request is more than the actual number because other stuff like json response parsing is also involved in CustomRestClient; and it is not scalable because we have to record metrics in each place where CustomRestClient. getInstanceStoppableCheck() is called.

This PR implements option 2), because there are not many places that call CustomRestClient APIs, and it doesn't look good that pass the namespace as a parameter just for metrics rather than logic related.

image

Tests

  • The following tests are written for this issue:

(List the names of added unit/integration tests)

  • The following is the result of the "mvn test" command on the appropriate module:
[INFO] Tests run: 168, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 59.766 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 168, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:05 min
[INFO] Finished at: 2020-09-28T17:43:10-07:00
[INFO] ------------------------------------------------------------------------

Documentation (Optional)

  • In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Commits

  • My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Code Quality

  • My diff has been formatted using helix-style.xml
    (helix-style-intellij.xml if IntelliJ IDE is used)

@huizhilu huizhilu force-pushed the custom-metrics branch 2 times, most recently from 46d54e3 to 5a07b17 Compare September 21, 2020 03:50
@huizhilu huizhilu self-assigned this Sep 21, 2020
@huizhilu huizhilu marked this pull request as ready for review September 22, 2020 02:15
@huizhilu huizhilu force-pushed the custom-metrics branch 3 times, most recently from 13c1079 to f497030 Compare September 29, 2020 01:32
Copy link
Contributor Author

@huizhilu huizhilu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zhangmeng916 @jiajunwang for the review! I've updated the PR to pass the tests and adjust the code.

Copy link
Contributor

@kaisun2000 kaisun2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff generally looks ok to me. Approve for now.

There are some comments JJ, please sync with him.

Copy link

@alirezazamani alirezazamani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@huizhilu
Copy link
Contributor Author

Thanks, all, for the review!

This PR is ready to be merged, approved by @kaisun2000 @alirezazamani

Custom rest client interacts with participant side to query for its health checks. Currently there is no metrics for the query requests. We'd like to have metrics for the external http requests.

This commit adds http request latency, requests total and error http request total metrics for the custom rest client calls.

@huizhilu huizhilu merged commit 26c026b into apache:master Sep 30, 2020
@huizhilu huizhilu deleted the custom-metrics branch September 30, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add metrics for custom rest client request

5 participants