Skip to content

Conversation

sezal98
Copy link
Contributor

@sezal98 sezal98 commented Dec 30, 2024

Why make this change?

Design Document for Health Endpoint
Resolves: 2504

What is this change?

Adding a design doc to help contributors to get an understanding of how the end to end health endpoint is coded and tested

  • Added coding specifics, functions and conditions
  • Added Test scenarios

@abhishekkumams
Copy link
Contributor

How would the health endpoint will function with hot reload? In case of bad config it might use last good known config which might give wrong status.

Would be good to add limitations.

@sezal98
Copy link
Contributor Author

sezal98 commented Jan 3, 2025

How would the health endpoint will function with hot reload? In case of bad config it might use last good known config which might give wrong status.

Would be good to add limitations.

I dont see an issue in this, If DAB Engine is working then health point should be consistent with that. If DAB engine is using the last known config, then health endpoint would also show the health of the last known config.
Showing health of the current config but running the engine with the last known config would create inconsistencies which would be incorrect for the user.
We should put this in documentation that we are using the last known config in case of hot reload and showing health of that old config not the bad config.

Another important point to note is.. Health is independent of Validity of config. Health endpoint is only shown when the config is valid (no errors) and DAB engine is running. So the case of bad config would not occur.

@JerryNixon , Any thoughts on this?

@abhishekkumams
Copy link
Contributor

How would the health endpoint will function with hot reload? In case of bad config it might use last good known config which might give wrong status.
Would be good to add limitations.

I dont see an issue in this, If DAB Engine is working then health point should be consistent with that. If DAB engine is using the last known config, then health endpoint would also show the health of the last known config. Showing health of the current config but running the engine with the last known config would create inconsistencies which would be incorrect for the user. We should put this in documentation that we are using the last known config in case of hot reload and showing health of that old config not the bad config.

Another important point to note is.. Health is independent of Validity of config. Health endpoint is only shown when the config is valid (no errors) and DAB engine is running. So the case of bad config would not occur.

@JerryNixon , Any thoughts on this?

Thanks for clarifying. can you add the same to your documentation as well.

@abhishekkumams
Copy link
Contributor

How will we be checking if an endpoint is healthy or not? Are we gonna make a db call internally? Or hit the endpoint itself to check the status? Or something else?

@sezal98
Copy link
Contributor Author

sezal98 commented Jan 9, 2025

How will we be checking if an endpoint is healthy or not? Are we gonna make a db call internally? Or hit the endpoint itself to check the status? Or something else?

Yes, as mentioned in the document, we would be making the call internally and hitting the specified query. If this query is sucessfull and within the threshold specified, the entity would be considered healthy. Else, we would say, its unhealthy with the exception message

@Aniruddh25
Copy link
Collaborator

How will we be checking if an endpoint is healthy or not? Are we gonna make a db call internally? Or hit the endpoint itself to check the status? Or something else?

Yes, as mentioned in the document, we would be making the call internally and hitting the specified query. If this query is sucessfull and within the threshold specified, the entity would be considered healthy. Else, we would say, its unhealthy with the exception message

What if the specified query itself has Syntax errors? if the datasource is healthy, we might report a false -ve. how do we plan to validate the query text?

@Aniruddh25
Copy link
Collaborator

Fix the PR description to remove the text from the PR template

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Waiting for addressing the comments

sezal98 and others added 2 commits January 15, 2025 22:23
Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
Copy link
Contributor

@JerryNixon JerryNixon left a comment

Choose a reason for hiding this comment

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

@sezal98 sezal98 requested a review from JerryNixon February 17, 2025 11:39
@sezal98
Copy link
Contributor Author

sezal98 commented Feb 17, 2025

/azp run

Copy link
Contributor

@abhishekkumams abhishekkumams left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for updating.

@sezal98
Copy link
Contributor Author

sezal98 commented Feb 18, 2025

/azp run

1 similar comment
@sezal98
Copy link
Contributor Author

sezal98 commented Feb 18, 2025

/azp run

@sezal98
Copy link
Contributor Author

sezal98 commented Feb 19, 2025

/azp run

@sezal98 sezal98 requested a review from JerryNixon February 19, 2025 16:43
Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

This has come a long way! Thank you for your patience in addressing all the comments.

Left a few comments to answer but other than that looks good to merge.

@sezal98
Copy link
Contributor Author

sezal98 commented Feb 21, 2025

/azp run

@sezal98
Copy link
Contributor Author

sezal98 commented Feb 24, 2025

/azp run

@sezal98 sezal98 dismissed JerryNixon’s stale review February 24, 2025 07:33

Merging for now. Will address any more comments in further PRs

@sezal98 sezal98 enabled auto-merge (squash) February 24, 2025 07:33
@sezal98
Copy link
Contributor Author

sezal98 commented Feb 24, 2025

/azp run

@sezal98 sezal98 merged commit f2b4a24 into main Feb 24, 2025
7 checks passed
@sezal98 sezal98 deleted the sezalchug/designDocHealthEndpoint branch February 24, 2025 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Design Document for Health Endpoint

5 participants