-
Notifications
You must be signed in to change notification settings - Fork 1
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
WIP: health/ endpoint #402
Conversation
[CHRON-450](https://blockchaintp.atlassian.net/browse/CHRON-450) Signed-off-by: Joseph Livesey <[email protected]>
[CHRON-425](https://blockchaintp.atlassian.net/browse/CHRON-425) Signed-off-by: Joseph Livesey <[email protected]>
@@ -522,6 +524,29 @@ impl SecurityConf { | |||
} | |||
} | |||
|
|||
pub struct Endpoints { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a set of enum values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That's just me thinking aloud, it's fine if you prefer it as it is, feel free to resolve this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not all mutually exclusive or related, but I agree seeing more than 2 bools at once is a worry. We may be pushing complexity up to configuration here. Simple data / graphql are always reasonable to assume. As they have known base urls, infrastructure can be used to exclude.
It's only health / metrics thats a choice here. Our current requirement is health, we can deal with exposing a prometheus endpoint properly later.
This is my fault, as I thought a prometheus endpoint would be basically free if we based health on metrics - apparently not the case but we will be able to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, I'm happy for none of this to be figured out in this PR if we'd prefer a separate ticket for endpoint conf (or not), at least it's just simple on-off configuration for separable things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was simply because the compiler was complaining about too many args in a function so I put them in a struct together.
@@ -522,6 +524,29 @@ impl SecurityConf { | |||
} | |||
} | |||
|
|||
pub struct Endpoints { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not all mutually exclusive or related, but I agree seeing more than 2 bools at once is a worry. We may be pushing complexity up to configuration here. Simple data / graphql are always reasonable to assume. As they have known base urls, infrastructure can be used to exclude.
It's only health / metrics thats a choice here. Our current requirement is health, we can deal with exposing a prometheus endpoint properly later.
This is my fault, as I thought a prometheus endpoint would be basically free if we based health on metrics - apparently not the case but we will be able to figure it out.
WIP: CHRON-450
poem
Route::at
alongside other--offer-endpoints
(currentlydata
andgraphql
) so it listens alongside them on a/metrics
path offAPI_LISTEN_SOCKET
- takes care of CHRON-450health/
endpoint that check current metrics from depth charge and returns500
String
from rendered metricsPR Checklist
Please complete this checklist after opening your PR