feat: Support observe backend health status #225 #325

vicanso · 2024-07-13T11:17:41Z

This PR supports add callback function for observing upstream backend health status.
Issue: #225

vicanso · 2024-07-19T16:41:49Z

@eaufavor
I want to know how to get PR accepted?

andrewhavck · 2024-07-27T16:23:26Z

pingora-load-balancing/src/health_check.rs

+#[async_trait]
+pub trait HealthObserve {
+    /// This function is called When health status changes
+    async fn health_check_callback(&self, _target: &Backend, _healthy: bool);


Mind removing the underscores in the parameters?

andrewhavck · 2024-07-27T16:25:15Z

pingora-load-balancing/src/health_check.rs

@@ -24,13 +24,24 @@ use pingora_http::{RequestHeader, ResponseHeader};
 use std::sync::Arc;
 use std::time::Duration;

+#[async_trait]
+pub trait HealthObserve {
+    /// This function is called When health status changes


Update the doc comment to be /// Observes the health of a [Backend], can be used for monitoring purposes..

andrewhavck · 2024-07-27T16:27:14Z

pingora-load-balancing/src/health_check.rs

+#[async_trait]
+pub trait HealthObserve {
+    /// This function is called When health status changes
+    async fn health_check_callback(&self, _target: &Backend, _healthy: bool);


Let's rename this function from health_check_callback to observe.

andrewhavck · 2024-07-27T16:28:25Z

pingora-load-balancing/src/health_check.rs

 /// [HealthCheck] is the interface to implement health check for backends
 #[async_trait]
 pub trait HealthCheck {
    /// Check the given backend.
    ///
    /// `Ok(())`` if the check passes, otherwise the check fails.
    async fn check(&self, target: &Backend) -> Result<()>;
+
+    /// This function is called When health status changes


Update doc comment to /// Called when the health changes for a [Backend]..

andrewhavck · 2024-07-27T16:29:20Z

pingora-load-balancing/src/health_check.rs

@@ -56,6 +67,7 @@ pub struct TcpHealthCheck {
    /// set, it will also try to establish a TLS connection on top of the TCP connection.
    pub peer_template: BasicPeer,
    connector: TransportConnector,
+    callback: Option<HealthCheckCallback>,


This can probably be pub and renamed to health_changed_callback. Doc comment can be /// A callback that is invoked when the `healthy` status changes for a [Backend].

andrewhavck · 2024-07-27T16:46:17Z

pingora-load-balancing/src/health_check.rs

@@ -93,6 +106,11 @@ impl TcpHealthCheck {
    pub fn set_connector(&mut self, connector: TransportConnector) {
        self.connector = connector;
    }
+
+    /// Set the health observe callback function.
+    pub fn set_callback(&mut self, callback: HealthCheckCallback) {


Can remove this if we change the callback to be pub.

andrewhavck · 2024-07-27T16:47:15Z

pingora-load-balancing/src/health_check.rs

        }
    }

    /// Replace the internal http connector with the given [HttpConnector]
    pub fn set_connector(&mut self, connector: HttpConnector) {
        self.connector = connector;
    }
+    /// Set the health observe callback function.
+    pub fn set_callback(&mut self, callback: HealthCheckCallback) {


Can remove this if we change the callback to be pub.

andrewhavck · 2024-07-27T16:47:39Z

pingora-load-balancing/src/health_check.rs

@@ -147,6 +171,7 @@ pub struct HttpHealthCheck {
    /// Sometimes the health check endpoint lives one a different port than the actual backend.
    /// Setting this option allows the health check to perform on the given port of the backend IP.
    pub port_override: Option<u16>,
+    callback: Option<HealthCheckCallback>,


This can probably be pub and renamed to health_changed_callback. Doc comment can be /// A callback that is invoked when the `healthy` status changes for a [Backend]...

andrewhavck · 2024-07-27T16:50:12Z

pingora-load-balancing/src/health_check.rs

+    /// This function is called When health status changes
+    async fn health_check_callback(&self, _target: &Backend, _healthy: bool);
+}
+pub type HealthCheckCallback = Box<dyn HealthObserve + Send + Sync>;


Let's rename this to HealthObserveCallback and add doc comment /// Provided to a [HealthCheck] to observe changes to [Backend] health.

andrewhavck · 2024-07-27T17:06:56Z

pingora-load-balancing/src/lib.rs

@@ -226,8 +226,10 @@ impl Backends {
                    h.observe_health(errored.is_none(), check.health_threshold(errored.is_none()));
                if flipped {
                    if let Some(e) = errored {
+                        check.health_status_change(backend, false).await;


Let's check.health_status_change(backend, errored.is_none()).await; before the if let check, then we can remove the redundant call.

vicanso · 2024-07-28T02:12:21Z

@andrewhavck I have modified the code, please review.

andrewhavck · 2024-07-30T20:13:49Z

@vicanso thanks for addressing the review comments, looks like there are some merge conflicts now? Once those are resolved I'll get this in internal review.

vicanso · 2024-08-08T14:44:07Z

@andrewhavck Is any adjustment needed?

--- test: add test for upstream health observe --- renamed the function and added doc to make it intelligible --- fix clippy error --- Merge branch 'main' into main --- test: fix test for backend do update Co-authored-by: Tree Xie <tree.xie@outlook.com> Includes-commit: 1421c26 Includes-commit: 695d549 Includes-commit: 6a09b52 Includes-commit: 72d6ee0 Includes-commit: e6c2af0 Includes-commit: fb62869 Replicated-from: #325

andrewhavck · 2024-08-09T21:37:17Z

@vicanso thanks for your contribution, this was included in our latest sync!

--- test: add test for upstream health observe --- renamed the function and added doc to make it intelligible --- fix clippy error --- Merge branch 'main' into main --- test: fix test for backend do update Co-authored-by: Tree Xie <tree.xie@outlook.com> Includes-commit: 1421c26 Includes-commit: 695d549 Includes-commit: 6a09b52 Includes-commit: 72d6ee0 Includes-commit: e6c2af0 Includes-commit: fb62869 Replicated-from: cloudflare#325

vicanso added 2 commits July 13, 2024 16:34

Support observe backend health status cloudflare#225

fb62869

test: add test for upstream health observe

e6c2af0

eaufavor added the enhancement New feature or request label Jul 26, 2024

andrewhavck self-assigned this Jul 26, 2024

andrewhavck requested changes Jul 27, 2024

View reviewed changes

vicanso added 2 commits July 28, 2024 09:46

renamed the function and added doc to make it intelligible

6a09b52

fix clippy error

1421c26

vicanso requested a review from andrewhavck July 30, 2024 14:14

vicanso added 2 commits July 31, 2024 07:34

Merge branch 'main' into main

695d549

test: fix test for backend do update

72d6ee0

andrewhavck closed this Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support observe backend health status #225 #325

feat: Support observe backend health status #225 #325

vicanso commented Jul 13, 2024

vicanso commented Jul 19, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

andrewhavck Jul 27, 2024

vicanso commented Jul 28, 2024

andrewhavck commented Jul 30, 2024

vicanso commented Aug 8, 2024

andrewhavck commented Aug 9, 2024

feat: Support observe backend health status #225 #325

feat: Support observe backend health status #225 #325

Conversation

vicanso commented Jul 13, 2024

vicanso commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicanso commented Jul 28, 2024

andrewhavck commented Jul 30, 2024

vicanso commented Aug 8, 2024

andrewhavck commented Aug 9, 2024