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 stats.atSendCapacity #603

Merged
merged 16 commits into from
Oct 8, 2024
Merged

Add stats.atSendCapacity #603

merged 16 commits into from
Oct 8, 2024

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented May 21, 2024

Fixes #559. For discussion.


Preview | Diff

@jan-ivar
Copy link
Member Author

These are modeled on estimatedSendRate, the lone stat specific to the WebTransport session, as opposed to all the other stats which are for the entire underlying connection.

I dunno if that was a mistake or not. We should discuss.

@jan-ivar jan-ivar changed the title Add stats.isSendDataLimited and stats.inSlowStart Add stats.sendRate.estimate + isDataLimited + isServerLimited and inSlowStart Jun 3, 2024
@jan-ivar
Copy link
Member Author

jan-ivar commented Jun 4, 2024

Meeting:

  • take out "datagrams" from isServerLimited
  • Maybe add required to all four?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jan-ivar
Copy link
Member Author

jan-ivar commented Jul 2, 2024

Meeting:

  • interest in going back to flat list of members. Right now we have:
      stats.sendRate.estimate;
      stats.sendRate.isDataLimited;
      stats.sendRate.isServerLimited;
      stats.sendRate.inSlowStart;
    Add a second PR to consider a flat list of members instead:
      stats.estimatedSendRate;
      stats.isSendingDataLimited;
      stats.isSendingServerLimited;
      stats.inSendingSlowStart;
  • Instead of server limited, should (instead) count "BLOCKED frames"?
    stats.blockedFramesCount;

index.bs Outdated Show resolved Hide resolved
@jan-ivar
Copy link
Member Author

jan-ivar commented Jul 3, 2024

Personally, I'm actually partial to a flat hierarchy a bit, since a hierarchy doesn't really solve ambiguity that much due to the ?. operator in JS. Example:

This will throw if stats or stats.sendRate is undefined:

stats.sendRate.isDataLimited;

This result in undefined (which might interpreted as false by websites):

stats.sendRate?.isDataLimited;

@jan-ivar
Copy link
Member Author

jan-ivar commented Jul 3, 2024

@wilaw wrote in #607 (comment):

In the prior meeting, a comment was made that some of the proposed 'sending' stats may not be known, and that coercing a boolean value could cause a problem. Is this a real problem, or can robust defaults be chosen that will not mislead applications?

(replying here about stats we're still adding here)

If unavailable stats are mostly a problem at connection start, then defaults reflecting initial state should work.

stats.estimatedSendRate; //nullable
stats.isSendingDataLimited; // default false

A connection is inherently data limited to start, so this would naturally default to (start out as) true, unless we can think of a name for the inverse state.

stats.isSendingServerLimited; // default false

Is it possible for a session to be both data limited AND server limited? If I have two long-lived streams, one server limited (receiving back pressure from the server) and the other data limited, would both be true? How would I know which applies to which?

stats.inSendingSlowStart; // default false
stats.blockedFramesCount; // default 0

These seem fine to me.

@jan-ivar
Copy link
Member Author

jan-ivar commented Jul 16, 2024

Meeting:

  • no objections to flat
  • suggest leaving Blocked frames to a different issue.
    • then we can remove stats.isSendingServerLimited if it ends up being redundant

@jan-ivar jan-ivar changed the title Add stats.sendRate.estimate + isDataLimited + isServerLimited and inSlowStart Add stats.isSendDataLimited Sep 9, 2024
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jan-ivar
Copy link
Member Author

Meeting:

  • bikeshed: atSendCapacity = false
  • need to keep sending to stay there
  • Note: atSendCapacity is not intended to replace the back-pressure signal. It is intended to signal the confidence level that you are at the estimated send rate. Capacity may increase or decrease based on network conditions.

@martinthomson
Copy link
Member

Including what I typed into chat...

When false, it indicates that estimatedSendRate is not being fully utilized and could be an unreliable estimate of actual network capacity.

When true, it indicates that the data that the application sends is close to the estimatedSendRate. As long as that send rate is sustained, estimatedSendRate will adapt to network conditions and be a fair estimate of what capacity is available to the application.

@jan-ivar jan-ivar changed the title Add stats.isSendDataLimited Add stats.atSendCapacity Oct 8, 2024
index.bs Outdated Show resolved Hide resolved
@jan-ivar jan-ivar merged commit 3a324bf into w3c:main Oct 8, 2024
2 checks passed
@jan-ivar jan-ivar deleted the senddatalimited branch October 8, 2024 23:22
github-actions bot added a commit that referenced this pull request Oct 8, 2024
SHA: 3a324bf
Reason: push, by jan-ivar

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quality of a bandwidth estimate
2 participants