Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Health check endpoint ignores fee escalation: #3914

Closed
wants to merge 1 commit into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Aug 26, 2021

High Level Overview of Change

Changed the health check endpoint (https://[host]:[peerport]/health) to only consider and consume the portion of the load_factor that is limited to the server. This ignores any changes related to fee escalation. Fee escalation describes how busy the ledger is, not the server.

Also refactored that function to use jss labels instead of strings for JSON fields.

Context of Change

I was looking at the health of my server and was annoyed when it went into warning status solely with a load factor in the 500's because fee escalation kicked in.

It is my opinion that this is not correct or desired behavior. I may be wrong about that, so that discussion would be appropriate here.

Type of Change

  • [X ] Bug fix (non-breaking change which fixes an issue)
  • [X ] Refactor (non-breaking change that only restructures code)

Before / After

Before: If the server's load_factor goes over 100 because of fee escalation, the /health endpoint will return a 503 result code and include the load_factor in the result JSON.

After: If the server's load_factor goes over 100 because of fee escalation, but the server's internal load is under 100, the /health endpoint will return a 200, and not include load_factor in the JSON. If the internal load goes over 100 because the server itself is busy, it will continue to be flagged by /health as before.

* Also refactor to use `jss` labels instead of strings for JSON fields.
Copy link
Contributor

@manojsdoshi manojsdoshi left a comment

Choose a reason for hiding this comment

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

LGTM

@manojsdoshi manojsdoshi added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 8, 2021
@manojsdoshi manojsdoshi mentioned this pull request Sep 9, 2021
@nbougalis nbougalis mentioned this pull request Sep 9, 2021
@ximinez ximinez deleted the health_load_factor branch September 15, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Bug Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants