fix(subscriber): Don't save poll_ops if no-one is receiving them #501
Conversation
Do not record poll_ops if there are no current connected clients (watchers). Without this `Aggregator::poll_ops` would grow forever. Follow up to tokio-rs#311 and fix for these two: - tokio-rs#184 - tokio-rs#500
There was a problem hiding this comment.
overall, this change looks good to me, thanks for the fix!
my one concern before merging this is just to make sure that the change to import ordering won't be undone if someone else saves this file while using default rustfmt settings. if you don't mind replying to my comment about that, I'll be happy to merge this PR once that's addressed!
thanks again!
|
|
||
| use console_api as proto; | ||
| use proto::resources::resource; | ||
| use tokio::sync::{mpsc, Notify}; | ||
| use tracing_core::{span::Id, Metadata}; | ||
|
|
||
| use super::{Command, Event, Shared, Watch}; | ||
| use crate::{ | ||
| stats::{self, Unsent}, | ||
| ToProto, WatchRequest, | ||
| }; | ||
|
|
There was a problem hiding this comment.
this is a minor nit, but were these imports reordered by rustfmt or some other editor feature? and if so, can you ensure that you're using the default rustfmt settings? i'm open to merging style changes such as reordering imports, but would prefer to make them in separate PRs from those that make functional changes, and to ensure that if someone else edits this file with the default rustfmt settings, that the change won't be undone. this helps to minimize churn in Git diffs.
thanks!
There was a problem hiding this comment.
AFAICT yes this is default rustfmt.
I was confused too by this. I have rustfmt 1.7.0-nightly (a577704 2023-11-16). It definitely isn't loading a .rustfmt.toml (or rustfmt.toml). I straced it and it tries a lot of different paths, all of them are "No such file or directory". I ran rustfmt from stable too. Same result.
Originally I was going to leave that part our of the PR, but I figured if it really is default rustfmt, it should be included.
Let me know if you want me to remove it.
|
@hawkw Excuse me, when is this PR expected to be merged? |
…scriber-v0.3.0 (#557) ## 🤖 New release * `tokio-console`: 0.1.10 -> 0.1.11 * `console-api`: 0.6.0 -> 0.7.0 * `console-subscriber`: 0.2.0 -> 0.3.0 ## `tokio-console` ## tokio-console-v0.1.11 - (2024-06-10) ### Added - Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa)) - Add flags and configurations for warnings ([#493](#493)) ([174883f](174883f)) - Add `--allow` flag ([#513](#513)) ([8da7037](8da7037)) ### Documented - Add note about running on Windows ([#510](#510)) ([a0d20fd](a0d20fd)) ### Fixed - Ignore key release events ([#468](#468)) ([715713a](715713a)) - Accept only `file://`, `http://`, `https://` URI ([#486](#486)) ([031bddd](031bddd)) - Fix column sorting in resources tab ([#491](#491)) ([96c65bd](96c65bd)) - Only trigger lints on async tasks ([#517](#517)) ([4593222](4593222)) - Remove duplicate controls from async ops view ([#519](#519)) ([f28ba4a](f28ba4a)) - Add pretty format for 'last woken' time ([#529](#529)) ([ea11ad8](ea11ad8)) ## `console-api` ## console-api-v0.7.0 - (2024-06-10) ### <a id = "0.7.0-breaking"></a>Breaking Changes - **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.10.x. ### Documented - Fix typo in proto ([#472](#472)) ([2dd3559](2dd3559)) ### Updated - [**breaking**](#0.7.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c)) ## `console-subscriber` ## console-subscriber-v0.3.0 - (2024-06-10) ### <a id = "0.3.0-breaking"></a>Breaking Changes - **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.10.x. ### Added - Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa)) - Reduce retention period to fit in max message size ([#503](#503)) ([bd3dd71](bd3dd71)) - Support grpc-web and add `grpc-web` feature ([#498](#498)) ([4150253](4150253)) ### Documented - Add a grpc-web app example ([#526](#526)) ([4af30f2](4af30f2)) - Fix typo in doc comment ([#543](#543)) ([ff22678](ff22678)) ### Fixed - Don't save poll_ops if no-one is receiving them ([#501](#501)) ([1656c79](1656c79)) - Ignore metadata that is not a span or event ([#554](#554)) ([852a977](852a977)) ### Updated - [**breaking**](#0.3.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c))
Do not record poll_ops if there are no current connected clients (watchers). Without this
Aggregator::poll_opswould grow forever.Follow up to #311 and fix for these two:
console_subscriber#184