♻️ refactor: Migrate HealthChecker to v3 #2884
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Grammar correction.
* chore(encryptcookie)!: update default config docs(encryptcookie): enhance documentation and examples BREAKING CHANGE: removed the hardcoded "csrf_" from the Except. * docs(encryptcookie): reads or modifies cookies * chore(encryptcookie): csrf config example * docs(encryptcookie): md table spacing
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 4 to 5. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v4...v5) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* middleware/logger: Log client IP address by default. * Update doc.
…2751) * Revert "Revert ":bug: requestid.Config.ContextKey is interface{} (gofiber#2369)" (gofiber#2742)" This reverts commit 28be17f. * fix: request ContextKey default value condition Should check for `nil` since it is `any`. * fix: don't constrain middlewares' context-keys to strings `context` recommends using "unexported type" as context keys to avoid collisions https://pkg.go.dev/github.com/gofiber/fiber/v2#Ctx.Locals. The official go blog also recommends this https://go.dev/blog/context. `fiber.Ctx.Locals(key any, value any)` correctly allows consumers to use unexported types or e.g. strings. But some fiber middlewares constrain their context-keys to `string` in their "default config structs", making it impossible to use unexported types. This PR removes the `string` _constraint_ from all middlewares, allowing to now use unexported types as per the official guidelines. However the default value is still a string, so it's not a breaking change, and anyone still using strings as context keys is not affected.
Update app.md for indentation
…#2762) Bumps [github.com/google/uuid](https://github.com/google/uuid) from 1.4.0 to 1.5.0. - [Release notes](https://github.com/google/uuid/releases) - [Changelog](https://github.com/google/uuid/blob/master/CHANGELOG.md) - [Commits](google/uuid@v1.4.0...v1.5.0) --- updated-dependencies: - dependency-name: github.com/google/uuid dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v2...v3) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
changing default log output Closes gofiber#2729
fix wrong hooks signature
…ig when AllowedOrigins is empty (gofiber#2771)
* redirect with query params did not work, fix it and add test for it * redirect middleware - fix test typo
* ✨ feat: add liveness and readiness checkers * 📝 docs: add docs for liveness and readiness * ✨ feat: add options method for probe checkers * ✅ tests: add tests for liveness and readiness * ♻️ refactor: change default endpoint values * ♻️ refactor: change default value for liveness endpoint * 📝 docs: add return status for liveness and readiness probes * ♻️ refactor: change probechecker to middleware * 📝 docs: move docs to middleware session * ♻️ refactor: apply gofumpt formatting * ♻️ refactor: remove unused parameter * split config and apply a review * apply reviews and add testcases * add benchmark * cleanup * rename middleware * fix linter * Update docs and config values * Revert change to IsReady * Updates based on code review * Update docs to match other middlewares --------- Co-authored-by: Muhammed Efe Cetin <efectn@protonmail.com> Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com>
- add more Parser tests
fix default value to false in docs of QueryBool
# Conflicts: # .github/pull_request_template.md # .github/release-drafter.yml # app.go # ctx.go # docs/api/middleware/basicauth.md # docs/api/middleware/csrf.md # docs/api/middleware/encryptcookie.md # docs/api/middleware/keyauth.md # docs/api/middleware/logger.md # docs/api/middleware/requestid.md # go.mod # go.sum # middleware/adaptor/adaptor_test.go # middleware/basicauth/config.go # middleware/cors/cors_test.go # middleware/csrf/config.go # middleware/csrf/csrf.go # middleware/idempotency/idempotency.go # middleware/keyauth/config.go # middleware/logger/config.go # middleware/logger/logger.go # middleware/redirect/redirect.go # middleware/requestid/config.go # middleware/requestid/requestid_test.go
Removes an an extra level of indentation in line 51 of `keyauth.md` [here](https://github.com/gofiber/fiber/blob/v2/docs/api/middleware/keyauth.md?plain=1#L51)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/healthcheck.md (2 hunks)
- middleware/healthcheck/config.go (1 hunks)
- middleware/healthcheck/healthcheck.go (2 hunks)
- middleware/healthcheck/healthcheck_test.go (7 hunks)
Additional comments: 3
middleware/healthcheck/healthcheck.go (1)
- 20-29: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [10-27]
The implementation of
NewHealthCheckerfunction correctly follows the new design of using a singleProbefunction for health checks. However, there are a few points to consider:
- The check for
c.Method() != fiber.MethodGetensures that only GET requests are processed by the middleware, which aligns with the typical use case for health checks. However, this might limit flexibility for users who might want to use other HTTP methods for health checks. Consider making the allowed HTTP methods configurable or documenting this behavior clearly.- The use of
c.SendStatusdirectly sends HTTP status codes based on the probe result, which is straightforward and efficient. Ensure that this aligns with the users' expectations in terms of response body content for health check endpoints.- It's good practice to include logging or metrics collection for health check failures to aid in debugging and monitoring the application's health status.
Overall, the changes are well-aligned with the objectives of simplifying and enhancing the middleware's functionality. Just ensure to consider the points mentioned for further refinement.
middleware/healthcheck/config.go (2)
- 19-19: The consolidation of
LivenessProbeandReadinessProbeinto a singleProbefield in theConfigstruct simplifies the configuration and usage of the healthcheck middleware. This change enhances flexibility by allowing users to define custom probes more easily. Ensure that the documentation and examples are updated to clearly demonstrate how to use the newProbefield for both liveness and readiness checks.- 27-39: The implementation of
defaultProbeas a function that always returnstrueis a sensible default behavior, indicating that the application is healthy by default. ThedefaultConfigV3function correctly handles the case where no custom configuration is provided by the user, applying this default probe. Additionally, it ensures that aProbeis always set, preventing potential nil pointer dereferences. This approach maintains the simplicity and safety of the middleware's configuration. Good job on ensuring that the default behavior is both logical and user-friendly.
|
Seems like it wasn't only healthchecker that suffered from this: |
Thats fine, we can address that in #2864 I'm more concern with the tests failing |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- middleware/healthcheck/healthcheck_test.go (8 hunks)
Additional comments: 4
middleware/healthcheck/healthcheck_test.go (4)
- 38-39: The addition of explicit routes
/livezand/readyzwithNewHealthChecker()inTest_HealthCheck_Strict_Routing_Defaultis a good practice. It ensures that the healthcheck middleware is tested under strict routing conditions, which is crucial for applications that require precise path matching. This change aligns with the PR's objective to enhance customization and simplify configuration.- 53-54: Replacing generic middleware usage with specific GET routes for
/livezand/readyzinTest_HealthCheck_Defaultis a significant improvement. It directly tests the middleware's response to these endpoints, ensuring that the healthcheck functionality is accessible and behaves as expected under default routing settings. This change contributes to the overall goal of simplifying the middleware's configuration and usage.- 91-91: The assertions for
StatusMethodNotAllowedin POST requests to/liveand/readyendpoints are crucial for verifying that the middleware correctly restricts HTTP methods. This ensures that the healthcheck endpoints adhere to best practices by only responding to GET requests, aligning with the principle of using appropriate HTTP methods for specific actions. This test effectively validates the middleware's method filtering capabilities.Also applies to: 96-96
- 112-118: In
Test_HealthCheck_Custom_Nested, the tests for nested paths/probe/liveand/probe/readyare well-structured and provide valuable coverage for applications that might structure their healthcheck endpoints within a nested path. This flexibility is a key aspect of the middleware's enhanced customization capabilities. Including tests for both successful and unsuccessful probes in nested paths ensures the middleware's behavior is predictable in various configurations.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- middleware/healthcheck/healthcheck_test.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/healthcheck/healthcheck_test.go
|
Performance Diff between Sequential / Parallel: |
what was the performance in the v2 branch ? its maybe related to my fix 5a0167a |
|
@ReneWerner87 It was around 120 ns/op according to this comment: #2863 (comment) But the performance before the commit and with the migration was around 110 ns/op according to this PR. I'll try and run it again with @gaby commits |
Looks the same as before to me, but since @gaby said it can be addressed in #2864 which also has other affected benchmarks |
|
@luk3skyw4lker It's also 3-4x faster in the Parallel benchmarks which simulates real world concurrent users. Something about single sequential requests slowed down in V3 |
|
@gaby yep, noticed that too, I'm gonna be taking care of some code rabbit comments right now and the PR should be good to go, do you guys have any other ideas for changes/improvements? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/api/middleware/healthcheck.md (2 hunks)
- middleware/healthcheck/config.go (2 hunks)
- middleware/healthcheck/healthcheck_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- docs/api/middleware/healthcheck.md
Additional comments: 7
middleware/healthcheck/config.go (2)
- 29-29: The
defaultProbefunction provides a sensible default behavior by always returningtrue. This is a good practice for a health check middleware, ensuring that the service is considered healthy by default unless explicitly configured otherwise.- 31-41: The
defaultConfigV3function correctly handles the case where no configuration is provided by the user, applying a default probe. This approach ensures backward compatibility and ease of use for developers integrating the health check middleware into their applications.middleware/healthcheck/healthcheck_test.go (5)
- 38-39: Adding explicit routes for
/livezand/readyzwithNewHealthChecker()in theTest_HealthCheck_Strict_Routing_Defaultfunction is a good practice. It ensures that the health check endpoints are correctly set up and respond as expected under strict routing configurations.- 53-54: The updates in
Test_HealthCheck_Defaultto useNewHealthChecker()for/livezand/readyzendpoints are appropriate and reflect the middleware's intended usage. This ensures that the health check functionality works as expected with the framework's default routing behavior.- 91-96: The tests for method restrictions (POST requests resulting in
StatusMethodNotAllowed) are important for ensuring that the health check endpoints adhere to the intended HTTP method constraints. This helps in maintaining the security and integrity of the health check mechanism.- 112-118: The
Test_HealthCheck_Custom_Nestedfunction correctly tests health check endpoints with nested paths, ensuring that the middleware functions correctly in more complex routing scenarios. This is crucial for applications that may organize health checks within a nested route structure.- 153-163: The use of the
Nextfunction inTest_HealthCheck_Nextto simulate a scenario where the middleware is skipped is well-implemented. This test ensures that the middleware behaves as expected when theNextfunction condition is met, providing flexibility in routing decisions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- middleware/healthcheck/healthcheck_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/healthcheck/healthcheck_test.go
…thchecker # Conflicts: # ctx_interface.go # docs/api/middleware/healthcheck.md # log/default_test.go # middleware/healthcheck/config.go # middleware/healthcheck/healthcheck.go # middleware/healthcheck/healthcheck_test.go
ReneWerner87
approved these changes
Feb 29, 2024
grivera64
added a commit
to grivera64/fiber
that referenced
this pull request
Mar 16, 2024
* Update pull_request_template.md * Update v3-changes.md * Update CONTRIBUTING.md (gofiber#2752) Grammar correction. * chore(encryptcookie)!: update default config (gofiber#2753) * chore(encryptcookie)!: update default config docs(encryptcookie): enhance documentation and examples BREAKING CHANGE: removed the hardcoded "csrf_" from the Except. * docs(encryptcookie): reads or modifies cookies * chore(encryptcookie): csrf config example * docs(encryptcookie): md table spacing * build(deps): bump actions/setup-go from 4 to 5 (gofiber#2754) Bumps [actions/setup-go](https://github.com/actions/setup-go) from 4 to 5. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v4...v5) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * 🩹 middleware/logger/: log client IP address by default (gofiber#2755) * middleware/logger: Log client IP address by default. * Update doc. * fix: don't constrain middlewares' context-keys to strings 🐛 (gofiber#2751) * Revert "Revert ":bug: requestid.Config.ContextKey is interface{} (gofiber#2369)" (gofiber#2742)" This reverts commit 28be17f. * fix: request ContextKey default value condition Should check for `nil` since it is `any`. * fix: don't constrain middlewares' context-keys to strings `context` recommends using "unexported type" as context keys to avoid collisions https://pkg.go.dev/github.com/gofiber/fiber/v2#Ctx.Locals. The official go blog also recommends this https://go.dev/blog/context. `fiber.Ctx.Locals(key any, value any)` correctly allows consumers to use unexported types or e.g. strings. But some fiber middlewares constrain their context-keys to `string` in their "default config structs", making it impossible to use unexported types. This PR removes the `string` _constraint_ from all middlewares, allowing to now use unexported types as per the official guidelines. However the default value is still a string, so it's not a breaking change, and anyone still using strings as context keys is not affected. * 📚 Update app.md for indentation (gofiber#2761) Update app.md for indentation * build(deps): bump github.com/google/uuid from 1.4.0 to 1.5.0 (gofiber#2762) Bumps [github.com/google/uuid](https://github.com/google/uuid) from 1.4.0 to 1.5.0. - [Release notes](https://github.com/google/uuid/releases) - [Changelog](https://github.com/google/uuid/blob/master/CHANGELOG.md) - [Commits](google/uuid@v1.4.0...v1.5.0) --- updated-dependencies: - dependency-name: github.com/google/uuid dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github/codeql-action from 2 to 3 (gofiber#2763) Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v2...v3) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Changing default log output (gofiber#2730) changing default log output Closes gofiber#2729 * Update hooks.md fix wrong hooks signature * 🩹 Fix: CORS middleware should use the defined AllowedOriginsFunc config when AllowedOrigins is empty (gofiber#2771) * 🐛 [Bug]: Adaptator + otelfiber issue gofiber#2641 (gofiber#2772) * 🩹🚨 - fix for redirect with query params (gofiber#2748) * redirect with query params did not work, fix it and add test for it * redirect middleware - fix test typo * ♻️ logger/middleware colorize logger error message gofiber#2593 (gofiber#2773) * ✨ feat: add liveness and readiness checks (gofiber#2509) * ✨ feat: add liveness and readiness checkers * 📝 docs: add docs for liveness and readiness * ✨ feat: add options method for probe checkers * ✅ tests: add tests for liveness and readiness * ♻️ refactor: change default endpoint values * ♻️ refactor: change default value for liveness endpoint * 📝 docs: add return status for liveness and readiness probes * ♻️ refactor: change probechecker to middleware * 📝 docs: move docs to middleware session * ♻️ refactor: apply gofumpt formatting * ♻️ refactor: remove unused parameter * split config and apply a review * apply reviews and add testcases * add benchmark * cleanup * rename middleware * fix linter * Update docs and config values * Revert change to IsReady * Updates based on code review * Update docs to match other middlewares --------- Co-authored-by: Muhammed Efe Cetin <efectn@protonmail.com> Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com> * prepare release v2.52.0 - add more Parser tests * fix healthcheck.md * configure workflows for V2 branch * configure workflows for V2 branch * Fix default value to false in docs of QueryBool (gofiber#2811) fix default value to false in docs of QueryBool * update queryParser config * Update ctx.md * Update routing.md * merge v2 in v3 * merge v2 in v3 * lint fixes * 📚 Doc: Fix code snippet indentation in /docs/api/middleware/keyauth.md Removes an an extra level of indentation in line 51 of `keyauth.md` [here](https://github.com/gofiber/fiber/blob/v2/docs/api/middleware/keyauth.md?plain=1#L51) * fix: healthcheck middleware not working with route group (gofiber#2863) * fix: healthcheck middleware not working with route group * perf: change verification method to improve perf * Update healthcheck_test.go * test: add not matching route test for strict routing * add more test cases * correct tests * correct test helpers * correct tests * correct tests --------- Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: René Werner <rene@gofiber.io> * merge v2 in v3 * Merge pull request from GHSA-fmg4-x8pw-hjhg * Enforce Wildcard Origins with AllowCredentials check * Expand unit-tests, fix issues with subdomains logic, update docs * Update cors.md * Added test using localhost, ipv4, and ipv6 address * improve documentation markdown --------- Co-authored-by: René Werner <rene@gofiber.io> * Update app.go prepare release v2.52.1 * fix cors domain normalize * fix sync-docs workflow * test: fix failing tests * fix sync-docs workflow * test: cors middleware use testify require * chore: fix lint warnings * chore: revert test isolation. * feat: migrate healthchecker to v3 * fix: use Get instead of Use for better router matching * docs: update docs to v3 info * fixed the fasthttp ctx race condition problem * Update middleware/cors/utils.go Co-authored-by: Renan Bastos <renanbastos.tec@gmail.com> * fix sync_docs.sh * fix review comments/hints * fix review comments/hints * stabilize Test_Proxy_Timeout_Slow_Server test * stabilize Test_Proxy_.* tests * ignore bodyclose linter for tests use http.NoBody instead of nil * Add parallel benchmark * Update healthcheck_test.go * docs: add comments for next behavior when there are no other handlers defined * revert(tests): undo http.NoBody usage --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: RW <rene@gofiber.io> Co-authored-by: tokelo-12 <113810058+tokelo-12@users.noreply.github.com> Co-authored-by: Jason McNeil <sixcolors@mac.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: iRedMail <2048991+iredmail@users.noreply.github.com> Co-authored-by: Benjamin Grosse <ste3ls@gmail.com> Co-authored-by: Mehmet Firat KOMURCU <mehmetfiratkomurcu@hotmail.com> Co-authored-by: Bruno <bdm2943@icloud.com> Co-authored-by: Muhammad Kholid B <muhammadkholidb@gmail.com> Co-authored-by: gilwo <gilwo@users.noreply.github.com> Co-authored-by: Muhammed Efe Cetin <efectn@protonmail.com> Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com> Co-authored-by: Jongmin Kim <kjongmin26@gmail.com> Co-authored-by: Giovanni Rivera <rivera.giovanni271@gmail.com> Co-authored-by: Renan Bastos <renanbastos.tec@gmail.com>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related to #2863
Changes Introduced
Migrating the healthchecker middleware to v3, using the proposition made by @ReneWerner87 in the above mentioned PR. Here is the benchmark and successful test results:
Type of Change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Summary by CodeRabbit