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

Indicate send blocking in data_moved events #444

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

Conversation

LPardue
Copy link
Member

@LPardue LPardue commented Nov 4, 2024

Fixes #132 (if merged).

It struck me that its the "data_moved" action that can be blocked, so rather than add
a separate event, just add an optional field to an existing event. This helps ensure we
can represent both stream and datagram data with all relevant fields.

Fixes #132 (if merged).

It struck me that its the "data_moved" action that can be blocked, so rather than add
a separate event, just add an optional field to an existing event. This helps ensure we
can represent both stream and datagram data with all relevant fields.
@rmarx
Copy link
Contributor

rmarx commented Nov 5, 2024

Conceptually this works, but I don't like the suggestion we'd have to omit logging length to indicate the data was actually not moved... in practice, length could be a strong signal that helps understand why exactly something was blocked for that reason (e.g., trying to write a ridiculous amount of data due to a bug).

If we keep this approach, I'd rather the guidance for this is "if there's a blocked_reason, that means the data was not actually moved, but a move was attempted, which was blocked due to blocked_reason". That imo also works :)
(that's also somewhat analogous to Nick's approach in the issue, saying "The lack of any flags means it's unblocked.")

@LPardue
Copy link
Member Author

LPardue commented Nov 5, 2024

Fair comment. For context, the quiche implementation has a stream write operation that takes an arbitrarily large buffer and can return a "partial write" result. For example, if the cwnd is 15 KB, and a 20 KB buffer is provided, well return an integer length result of 15 KB, indicating the app was blocked from writing the last 5 KB.

I was thinking it would be nice to reflect that in the qlog as a stream_data_moved with length 15 KB and blocked_reason of congestion window, etc.

I didn't think through everything in this proposal and maybe more guidance is useful, but it felt at the time of writing that combing the data in a single event would be more convenient for my use than if I had two separate events.

Always happy to hear other opinions or review alternatives though.

@kazuho
Copy link
Member

kazuho commented Nov 6, 2024

Having indications for blocked events is paramount to investigating performance issues.

Therefore, I think they have to be distinct events, rather than tied to data_moved events. That is because not all QUIC implementations "move" data between the layers at the same moment (consider implementations having different size of buffers, or having no buffer at all).

@nibanks
Copy link
Member

nibanks commented Nov 6, 2024

My personal preference is for separate events. We already have it built this way in msquic.

Also, I agree with Kazuho. We don't have any notion of moving data between layers.

@LPardue
Copy link
Member Author

LPardue commented Nov 6, 2024

Ack. Would someone be willing to help on the alternative proposal? Even if ypu can just frame how the alternative works, we can design the event structure definition around that.

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.

Add events to explicitly indicate send blocking
4 participants