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

feat: websocket for health #1256

Closed
wants to merge 6 commits into from
Closed

Conversation

tribhuwan-kumar
Copy link
Contributor

@tribhuwan-kumar tribhuwan-kumar commented Feb 1, 2025

feat: #1218 (comment)

maybe we have to update all the instance of fetchHealth & debouncedFetchHealth in all react components,
anyway i still kept these functions in health hook even tho they aren't necessary now

Copy link

vercel bot commented Feb 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
screenpipe ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 3, 2025 3:47pm

_app_handle: tauri::AppHandle,
tx: broadcast::Sender<crate::health::HealthCheckResponse>,
) -> Result<(), Box<dyn std::error::Error>> {
let addr = "127.0.0.1:9001";
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be risky port, maybe 14346? or can it be same port than the tauri server? not sure if it conflict with normal http server

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

async fn handle_socket(socket: WebSocket, query: Query<EventsQuery>) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm actually i have second thought

what about we do this health event on our existing event websocket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that'd be more easy!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about we do this health event on our existing event websocket?

async fn handle_socket(socket: WebSocket, query: Query<EventsQuery>) {

i tried to implement it but the problem is when screenpipe backend goes down the websocket server goes down too, cz the ws server is from backend,

.route("/ws/events", get(ws_events_handler))

after that its not possible to emit the health error which is something like this

    let error_health = HealthCheckResponse {
    status: "error".to_string(),
    last_frame_timestamp: None,
    last_audio_timestamp: None,
    last_ui_timestamp: None,
    frame_status: "error".to_string(),
    audio_status: "error".to_string(),
    ui_status: "error".to_string(),
    message: format!("health check failed: {}", e),
    status_code: 500,
    verbose_instructions: None,
};

one possible solution it'd be emit the error health before doing the shutdown, but it can create problem with react states!

Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess the hook would just show error when ws is disconnected

@tribhuwan-kumar
Copy link
Contributor Author

@louis030195
now the health websocket is on /ws/health endpoint of app's server

@louis030195
Copy link
Collaborator

@tribhuwan-kumar
Copy link
Contributor Author

i meant as custom event

https://github.com/mediar-ai/screenpipe/blob/main/screenpipe-events/src/events_manager.rs

yes, I tried it but I'm facing problems to getting values of error health

#1256 (comment)

@tribhuwan-kumar
Copy link
Contributor Author

i guess the hook would just show error when ws is disconnected

@louis030195
then what's the benefit of using webSocket if we have to keep connecting & disconnecting :(

@tribhuwan-kumar
Copy link
Contributor Author

tribhuwan-kumar commented Feb 6, 2025

@louis030195

any second thought?
also in windows it takes ~10s just to recognize is backend down or not this pr can fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants