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

Add health endpoint to event_display server #5608

Merged
merged 2 commits into from
Jul 26, 2021
Merged

Add health endpoint to event_display server #5608

merged 2 commits into from
Jul 26, 2021

Conversation

antoineco
Copy link
Contributor

@antoineco antoineco commented Jul 24, 2021

Proposed Changes

  • 🎁 Adds a health HTTP handler to event_display on the /healthz endpoint.

This handler responds with "204 No Content" to any request, regardless of the HTTP method. It is meant to be used for probing liveness/readiness.

Background: I observed race conditions on several occasions downstream, in e2e tests that rely on event_display. E.g. an event is sent as soon as the Pod starts, but the CE client isn't started yet 🤷.

I purposely avoided using something from k8s/knative to avoid large imports ("fat binary").

Pre-review Checklist

  • At least 80% unit test coverage (⚠️ event_display didn't have unit tests prior to this PR. I'll add more tests in a subsequent refactoring.)
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note

Add a health endpoint to `event_display` for enabling readiness probes.

Docs

@antoineco antoineco requested a review from matzew July 24, 2021 14:21
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2021
@knative-prow-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 24, 2021
@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 24, 2021
@codecov
Copy link

codecov bot commented Jul 24, 2021

Codecov Report

Merging #5608 (585c8c7) into main (7251c1b) will decrease coverage by 0.06%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5608      +/-   ##
==========================================
- Coverage   82.79%   82.73%   -0.07%     
==========================================
  Files         199      200       +1     
  Lines        6230     6243      +13     
==========================================
+ Hits         5158     5165       +7     
- Misses        744      748       +4     
- Partials      328      330       +2     
Impacted Files Coverage Δ
cmd/event_display/main.go 53.84% <58.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7251c1b...585c8c7. Read the comment docs.

@antoineco
Copy link
Contributor Author

Demo:

$ curl -D- http://localhost:8080/ -H "Ce-Specversion: 1.0" -H "Ce-Id: 0001" -H "Ce-Type: greeting" -H "Ce-Source: curl" -H "Content-Type: application/json" -d '{"msg":"Hello, World!"}
HTTP/1.1 200 OK
Date: Sat, 24 Jul 2021 14:33:09 GMT
Content-Length: 0
$ curl -D- http://localhost:8080/healthz
HTTP/1.1 204 No Content
Date: Sat, 24 Jul 2021 14:31:48 GMT

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 24, 2021
@antoineco antoineco marked this pull request as ready for review July 24, 2021 22:54
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2021
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/event_display/main.go Do not exist 63.6%

Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit a005619 into knative:main Jul 26, 2021
@antoineco antoineco deleted the ev-disp-healthz branch July 26, 2021 14:00
Comment on lines +62 to +64
c, err := cloudevents.NewClientHTTP(
cehttp.WithMiddleware(healthzMiddleware()),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@n3wscott I needed something quick so I used a Middleware for the time being, but this feels clumsy to me (see the implementation of healthzMiddleware() below which is especially hacky).

Since I'm going to refactor this for testability, I thought now was a good time to ask for your expertise :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants