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

Add events to explicitly indicate send blocking #132

Open
rmarx opened this issue Jan 20, 2021 · 8 comments · May be fixed by #444
Open

Add events to explicitly indicate send blocking #132

rmarx opened this issue Jan 20, 2021 · 8 comments · May be fixed by #444
Assignees

Comments

@rmarx
Copy link
Contributor

rmarx commented Jan 20, 2021

msquic has separate events that indicate if the sender was blocked/delayed/idle and due to which cause (e.g., pacing, flow control limited, cwnd limited, etc.).

This is quite useful for debugging. While some of this can be deduced from looking at other existing qlog events, adding some more explicit events for this could be interesting.

@nibanks: maybe you could give some pointers/links to where these events are defined for msquic for reference?

cc @LPardue

@LPardue
Copy link
Member

LPardue commented Jan 20, 2021

As far as I could intuit, MS had events for blocked and unblocked.

This seemed to be an application/ implementation event, rather than something strictly transport related. So I wonder what type of category it would fit into.

@nibanks
Copy link
Member

nibanks commented Jan 20, 2021

https://github.com/microsoft/msquic/blob/main/src/inc/quic_trace.h#L44

typedef enum QUIC_FLOW_BLOCK_REASON {
    QUIC_FLOW_BLOCKED_SCHEDULING            = 0x01,
    QUIC_FLOW_BLOCKED_PACING                = 0x02,
    QUIC_FLOW_BLOCKED_AMPLIFICATION_PROT    = 0x04,
    QUIC_FLOW_BLOCKED_CONGESTION_CONTROL    = 0x08,
    QUIC_FLOW_BLOCKED_CONN_FLOW_CONTROL     = 0x10,
    QUIC_FLOW_BLOCKED_STREAM_ID_FLOW_CONTROL= 0x20,
    QUIC_FLOW_BLOCKED_STREAM_FLOW_CONTROL   = 0x40,
    QUIC_FLOW_BLOCKED_APP                   = 0x80
} QUIC_FLOW_BLOCK_REASON;

The lack of any flags means it's unblocked.

@nibanks
Copy link
Member

nibanks commented Jan 20, 2021

And the connection has a set of flags, as well as each stream has its own set.

@LPardue
Copy link
Member

LPardue commented Jan 20, 2021

I guess my intuit was a bit off 😜

@LPardue
Copy link
Member

LPardue commented Sep 8, 2022

Leaning on the side of let folks log what they like, and focus these drafts on transport events.

@rmarx
Copy link
Contributor Author

rmarx commented Sep 29, 2022

I still personally feel these would be very useful to have.

I'm not sure why these wouldn't be part of "transport events" @LPardue? Seems like being blocked on e.g., flow control is a very transport-esque thing?

Right now, we can somewhat deduce this information by e.g., looking at logged values for MAX_STREAM_DATA and friends and *_BLOCKED and matching those with actual StreamFrame sent events, but having something explicit is useful here imo.

@rmarx rmarx added the discuss needs further discussion label Sep 29, 2022
@LPardue
Copy link
Member

LPardue commented Sep 29, 2022

There's no QUIC message to tell your peer you can't send it data due to pacing or amplification prevention. So to do that in qlog here, we'd have to invent some new category of event and explain to people what it means and how to use it (produce and consume).

Yes it could be interesting but I lean on punting it to be done when there's a good case about why it needs to be defined in an interoperable way in qlog.

@LPardue
Copy link
Member

LPardue commented Dec 9, 2023

I still think we can live without standardizing this today. It seems a generic "application data" category of event can be defined that would work for e.g. both HTTP/2 and HTTP/3 sending on a stream could be blocked due to stream flow control or the transport congestion control. That's simple enough to write in a new spec.

@rmarx rmarx assigned LPardue and rmarx and unassigned lnicco and LPardue Jun 24, 2024
LPardue added a commit that referenced this issue 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.
@LPardue LPardue linked a pull request Nov 4, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants