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: support sse transfer mode #122

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: support sse transfer mode #122

wants to merge 4 commits into from

Conversation

mzzsfy
Copy link

@mzzsfy mzzsfy commented Jan 5, 2024

support #117

@mzzsfy
Copy link
Author

mzzsfy commented Jan 5, 2024

Don't allow this PR, I will update this PR when the other 2 PR are passed,
and add some tests

@mzzsfy mzzsfy changed the title feat: sse transfer mode feat: support sse transfer mode Jan 5, 2024
@mzzsfy
Copy link
Author

mzzsfy commented Jan 5, 2024

When this function is stable, it should be possible to remove websocket dependencies.

@arl
Copy link
Owner

arl commented Jan 5, 2024

We should switch to SSE and not keep an option websockets/SSE. Has no use for the user

@mzzsfy
Copy link
Author

mzzsfy commented Jan 5, 2024

We should switch to SSE and not keep an option websockets/SSE. Has no use for the user

You are righ, I will have further updates after you merge other PR

@mzzsfy
Copy link
Author

mzzsfy commented Jan 6, 2024

I submitted new code without using cache and solved all problems #119 #117

export { init, pushData, slice };
Copy link
Author

Choose a reason for hiding this comment

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

Line breaks automatically added by ide🤣

@mzzsfy
Copy link
Author

mzzsfy commented Jan 15, 2024

I've noticed no movement for a week. Is there something wrong with this pr?

@arl
Copy link
Owner

arl commented Jan 15, 2024

I've noticed no movement for a week. Is there something wrong with this pr?

Hi. just haven't got the time yet

@mzzsfy
Copy link
Author

mzzsfy commented Jan 15, 2024

Okay, continue to wait for your news. Now I am using replace to run the code. After you merge it, I can officially switch over.

Copy link
Owner

@arl arl left a comment

Choose a reason for hiding this comment

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

This pull request does more than what is advertised. In particular, it also handles multiples clients at once, which is an entire topic entirely (#119).
Please keep the changes to the bare minimum, which is to switch from websocket to sse

@mzzsfy
Copy link
Author

mzzsfy commented Jan 20, 2024

This pull request does more than what is advertised. In particular, it also handles multiples clients at once, which is an entire topic entirely (#119). Please keep the changes to the bare minimum, which is to switch from websocket to sse

Now that's 2 different pr's.

@mzzsfy
Copy link
Author

mzzsfy commented Feb 26, 2024

It has been a month, have you encountered any difficulties?

Copy link
Owner

@arl arl left a comment

Choose a reason for hiding this comment

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

This PR does more than advertized and it makes it difficult to review.
I'd expect the minimum code to switch the communication from websockets to SSE, nothing more nothing less.
If some stuff needs to be changed because of the websockets-> SSe migration, then please say what and why.

Thank you

statsviz.go Outdated
@@ -82,7 +81,7 @@ func Register(mux *http.ServeMux, opts ...Option) error {
// updates metrics data and provides two essential HTTP handlers:
// - the Index handler serves Statsviz user interface, allowing you to
// visualize runtime metrics on your browser.
// - The Ws handler establishes a WebSocket connection allowing the connected
// - The Ws handler establishes a data connection allowing the connected
Copy link
Owner

Choose a reason for hiding this comment

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

The Ws handler is used in examples and is part of the public API, but since ws means websockets and the communication won't be performed via websockets anymore, the Ws handler must be renamed to SSE. Removing it would be a breaking change however.

So, we should keep the Ws handler and mark is as deprecated, and make it an alias of the SSE handler.

panic("unexpected failure to encode plot definitions: " + err.Error())
func intercept(h http.Handler, cfg *plot.Config, extraConfig map[string]any) http.HandlerFunc {
var plotsdefjs []byte
//Using parentheses helps gc
Copy link
Owner

Choose a reason for hiding this comment

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

//Using parentheses helps gc

I've never heard of that, can you point to an blog post or a benchmark or anything about that?

Comment on lines 114 to 138
function throttle(func, delay) {
let initial = true;
let last = null;
let timer = null;
return function () {
const context = this;
const args = arguments;
if (initial) {
func.apply(context, args);
initial = false;
last = Date.now();
} else {
clearTimeout(timer);
timer = setTimeout(function () {
const now = Date.now();
if (now - last >= delay) {
func.apply(context, args);
last = now;
}
}, delay - (Date.now() - last));
}
}
}

const updatePlots = throttle(() => {
Copy link
Owner

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

It can avoid repeated redrawing of DOM caused by browser energy saving mode. This will not affect normal users.
Described in #120

@@ -123,7 +156,7 @@ const updatePlots = () => {
plot.update(xrange, data, shapes);
}
});
}
}, PlotsDef.sendFrequency||1000)
Copy link
Owner

Choose a reason for hiding this comment

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

why is the sendFrequency setting used here? it wasn't before and I don't understand why it's needed now. Updates are pushed from the server, so I'd expect browser-side updates to be performed when data gets pushed.

Copy link
Author

Choose a reason for hiding this comment

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

This sets a minimum time for DOM redrawing, maybe I should set it smaller, such as 100

@mzzsfy
Copy link
Author

mzzsfy commented Feb 28, 2024

create a new method Metrics() and mark Ws() as deprecated,add method MetricsPath() for custom paths,Compatibility has been ensured as much as possible
updated all examples

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