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

Increase WebSocket skipped updates metric when queue is not fetched #106

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

diegodiv
Copy link
Contributor

@diegodiv diegodiv commented Mar 30, 2023

Fixes #103.

I reused the already existing metric, because the user-facing issue would be the same, and we can easily know in which case we are looking at the size of the queue (which can be computed from other metrics).

@diegodiv diegodiv requested a review from fatho March 30, 2023 10:10
Copy link
Member

@fatho fatho left a comment

Choose a reason for hiding this comment

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

I think it would be better to have a different metric for this, as ultimately we're counting different things here:

  • At this point, one count means one client didn't receive one update, and the cause can be narrowed down to the sending part.
  • For the other queue, one count means all clients didn't receive one update, and the cause is likely to be found elsewhere.

It's always easy to make an aggregate dashboard that shows both (or just the sum of both), but getting the individual information from a compound metric after the fact is often hard.

@diegodiv
Copy link
Contributor Author

I think I see your point. Though, a bit of detail: when we discard an update in the main update queue (which shouldn't ever happen, now), we don't know which client is affected, but only one of them will be, under our current use, and we're sure only one path will be affected.

However, I agree that it would make the information easier to see if we have two different metrics.

@fatho
Copy link
Member

fatho commented Mar 30, 2023

we don't know which client is affected, but only one of them will be

There still can be multiple clients subscribed to the same path, so while indeed it likely doesn't affect all of them if we lose one update to the coreUpdates queue, it's also not necessarily limited to a single client. There could also be no client at all listening on that path. All in all, that makes it even harder to assess the impact of a skipped update at that point.

@diegodiv diegodiv requested a review from fatho March 30, 2023 12:37
server/src/WebsocketServer.hs Show resolved Hide resolved
server/src/Metrics.hs Outdated Show resolved Hide resolved
@diegodiv diegodiv requested a review from fatho March 30, 2023 14:12
server/src/Metrics.hs Show resolved Hide resolved
@diegodiv diegodiv requested a review from fatho April 4, 2023 07:53
Copy link
Member

@fatho fatho left a comment

Choose a reason for hiding this comment

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

:shipit:

@diegodiv
Copy link
Contributor Author

diegodiv commented Apr 4, 2023

@OpsBotPrime merge

@OpsBotPrime
Copy link
Contributor

Pull request approved for merge by @DiegoDiverio, rebasing now.

Diego Diverio and others added 2 commits April 4, 2023 09:56
@OpsBotPrime
Copy link
Contributor

Rebased as 65259c1, waiting for CI …

@OpsBotPrime
Copy link
Contributor

CI job 🟡 started.

@OpsBotPrime OpsBotPrime merged commit 65259c1 into master Apr 4, 2023
@OpsBotPrime OpsBotPrime deleted the dd/subscriber-metrics branch April 4, 2023 08:09
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.

Expose metrics on subscribers
3 participants