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

Filterable Health Tags #1304

Merged
merged 13 commits into from
Apr 10, 2023
Merged

Filterable Health Tags #1304

merged 13 commits into from
Apr 10, 2023

Conversation

ceyonur
Copy link
Contributor

@ceyonur ceyonur commented Apr 6, 2023

Why this should be merged

  • Adds filterable string tags to health workers
  • Registers chain-related health checks with subnetIDs tags
  • Adds a global tag for those checks like db/network. Global health checks are always returned regardless of filters.
  • Adds tags optional param to Post request. Ex:
curl --location --request POST 'http://localhost:9660/ext/health' \
--header 'Content-Type: application/json' \
--data-raw '{
    "jsonrpc":"2.0",
    "id"     :1,
    "method" :"health.health",
    "params" :{
    "tags": ["29uVeLPJB1eQJkzRemU8g8wZDw5uJRqpab5U2mX9euieVwiEbL"]
    }

}'
  • Adds tag optional query param. Supports multiple tag params. Ex:
curl --location --request GET 'http://localhost:9660/ext/health?tag=29uVeLPJB1eQJkzRemU8g8wZDw5uJRqpab5U2mX9euieVwiEbL' \
--header 'Content-Type: application/json' \
--data-raw '{
    "jsonrpc":"2.0",
    "id"     :1,
    "method" :"health.health",
}'

Closes: #1264

How this works

expands the Health result/register interface by adding input of optional tags to Register and Result methods.
Tracks tags > names map in the worker

How this was tested

Added unit tests

@ceyonur ceyonur self-assigned this Apr 6, 2023
@ceyonur ceyonur linked an issue Apr 6, 2023 that may be closed by this pull request
@ceyonur ceyonur requested a review from joshua-kim April 6, 2023 16:56
@ceyonur ceyonur changed the title Subnet healths Filterable Health Tags Apr 6, 2023
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

Just small stuff

api/health/service.go Outdated Show resolved Hide resolved
api/health/service.go Outdated Show resolved Hide resolved
api/health/service.go Show resolved Hide resolved
api/health/service.go Outdated Show resolved Hide resolved
api/health/service.go Outdated Show resolved Hide resolved
api/health/handler.go Outdated Show resolved Hide resolved
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Make sure the content type is set before writing the header.
w.Header().Set("Content-Type", "application/json")

checks, healthy := reporter()
subnetIDs := r.URL.Query()["subnetID"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda weird that the key is subnetID and it returns subnetIDs... But I do think this is the best way to do this with the query params...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea and it's even weirder that r.URL.Query().Get("subnetID") returns the first value for the key... alternatively we can use plural subnetIDs (now tags) and split string value by comas. but I probably would prefer singular key names (i.e tag over tags).

@StephenButtolph StephenButtolph added this to the v1.10.0 (Cortina) milestone Apr 6, 2023
@StephenButtolph StephenButtolph added monitoring This primarily focuses on logs, metrics, and/or tracing incident response labels Apr 6, 2023
api/health/handler.go Outdated Show resolved Hide resolved
api/health/handler.go Outdated Show resolved Hide resolved
api/health/health_test.go Show resolved Hide resolved
api/health/service.go Outdated Show resolved Hide resolved
api/health/health_test.go Outdated Show resolved Hide resolved
api/health/service_test.go Outdated Show resolved Hide resolved
api/health/health.go Outdated Show resolved Hide resolved
Copy link

@danlaine danlaine left a comment

Choose a reason for hiding this comment

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

just a few more nits. Once these are done LGTM

api/health/service_test.go Outdated Show resolved Hide resolved
chains/manager.go Show resolved Hide resolved
api/health/health_test.go Outdated Show resolved Hide resolved
Copy link

@danlaine danlaine left a comment

Choose a reason for hiding this comment

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

1 final nit otherwise LGTM

api/health/service_test.go Outdated Show resolved Hide resolved
api/health/client.go Show resolved Hide resolved
chains/manager.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incident response monitoring This primarily focuses on logs, metrics, and/or tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API] Add Subnet-Specific Health Checks
4 participants