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: add temporality state to update message #567

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rustin170506
Copy link
Collaborator

@Rustin170506 Rustin170506 commented Jul 4, 2024

close #551

This PR added a new field to the console's update message. Clients can use it to check if the subscriber stream is paused.

For example:
image

@Rustin170506 Rustin170506 force-pushed the rustin-patch-state branch 3 times, most recently from 42f9bc6 to 73331fd Compare July 5, 2024 13:26
@Rustin170506 Rustin170506 marked this pull request as ready for review July 5, 2024 13:27
@Rustin170506 Rustin170506 requested a review from a team as a code owner July 5, 2024 13:27
Copy link
Collaborator Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

This is not a perfect solution to fully solve this problem.

It can only be used before the new client starts, for already running clients, it doesn't help, because there is no message sent to that second client.

@Rustin170506 Rustin170506 requested a review from hds July 5, 2024 13:39
@Rustin170506 Rustin170506 changed the title feat: add temporality state in update feat: add temporality state to update message Jul 5, 2024
@Rustin170506 Rustin170506 requested a review from hawkw July 6, 2024 02:07
This field can be used to check if the stream has been paused.
Copy link
Collaborator

@hds hds 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 that putting the pause/live state into a separate update stream would make it flexible enough to update all clients that the subscriber is paused.

@@ -5,6 +5,7 @@ use crate::{
warnings::Linter,
};
use console_api as proto;
use console_api::instrument::Temporality;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For all other items used from the console_api crate, we reference by the "full" path (but starting with proto), I think we should do the same here to not make Temporality different.

Suggested change
use console_api::instrument::Temporality;

@@ -107,6 +102,14 @@ impl State {
current_view: &view::ViewState,
update: proto::instrument::Update,
) {
match Temporality::try_from(update.temporality) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps best to stick to the way that we reference all other types from the console_api crate.

Suggested change
match Temporality::try_from(update.temporality) {
match proto::instrument::Temporality::try_from(update.temporality) {

}

// The time "state" of the aggregator.
enum Temporality {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would make sense to add another rpc for console subscriber state updates. Right now temporality would be the only thing in there, but this would allow us to do "out of band" state updates such as this case, where no data is being sent.

This would also allow us to solve the issue with already connected clients, as we could send this new server state update without needing to send a real update, so all connected tokio console instances would get the pause update.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good! It would be a good solution.

@Rustin170506
Copy link
Collaborator Author

Finally, I have some time to work on it again. I will try to add a new API for it.

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.

tokio-console cannot show the pause status correctly
2 participants