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

fix: statsviz.TimeSeries.GetValue is called multiple times per second #121

Closed
wants to merge 1 commit into from

Conversation

mzzsfy
Copy link

@mzzsfy mzzsfy commented Jan 4, 2024

close #119

@mzzsfy mzzsfy changed the title fix GitHub #119 fix: statsviz.TimeSeries.GetValue is called multiple times per second Jan 4, 2024
@arl
Copy link
Owner

arl commented Jan 6, 2024

Currently, there's a coupling between stats collection and stats emission, which means that if we have multiple clients to emit stats to, we collect stats multiple times. That's what we're trying to solve

I think there might be a simpler solution than what you're proposing in this PR, that doesn't require a new option nor even requires caching.

So, we want to stats collection from stats emission. By doing so, we'll have a single collection goroutine for N clients. To do this, we need to keep track of the currently connected clients.
Our single collection goroutine will push somehow the latest collected stats to N currently connected clients. No need for a cache

Aside from that, I think that we should remove websocket and switch to SSE before tacking the current PR, since the code will likely be different by then.

So, I really thank you for trying to solve it, but you see it'd be have been preferable to discuss it before doing the work and sending the PR. Also, somebody, myself included, could even have already been working on it. So I'm saying that since everybody, you included, time and effort is of great but limited value.

Again, I thank you for your excitement into contributing to the project and don't mean to tone it down, I just think we should coordinate, ok?

So to sum it up, the first thing to do should probably switch to SSE, since that would impact the 2 other PR's.
Then we should probably tackle the lagging, if we can still reproduce it, see #120 (comment).

Finally come back to the current PR.

@mzzsfy
Copy link
Author

mzzsfy commented Jan 6, 2024

Yes, your solution may be more suitable, I will put more effort into sse support

@mzzsfy
Copy link
Author

mzzsfy commented Jan 6, 2024

Already solved #122

@mzzsfy mzzsfy closed this Jan 6, 2024
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.

statsviz.TimeSeries.GetValue is called multiple times per second
2 participants