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 per-stream getStats(). #395

Merged
merged 10 commits into from
Apr 27, 2022
Merged

Add per-stream getStats(). #395

merged 10 commits into from
Apr 27, 2022

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Mar 29, 2022

Fixes #372.


Preview | Diff

@jan-ivar
Copy link
Member Author

@ylafon @dontcallmedom any idea why these checks are failing?

Error: w3c/spec-prod/v2/action.yml (Line: 113, Col: 24):
Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 113, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
DOMHighResTimeStamp timestamp;
unsigned long long amountWrittenTo;
unsigned long long sentProgress;
unsigned long long acknowledgedProgress;
Copy link
Member

Choose a reason for hiding this comment

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

bytesAcknowledged ?

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

Meeting:

index.bs Outdated Show resolved Hide resolved
@dontcallmedom
Copy link
Member

@ylafon @dontcallmedom any idea why these checks are failing?

Error: w3c/spec-prod/v2/action.yml (Line: 113, Col: 24):
Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 113, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.

The actual error is hidden above that bogus error message:

  LINK ERROR: No 'idl' refs found for '[[InternalStream]]'.

{{SendStream/[[InternalStream]]}}
✘ Did not generate, due to fatal errors

@ricea
Copy link
Contributor

ricea commented Mar 30, 2022

Subclassing looks okay to me.

The IDL for createUnidirectionalStream() needs to be updated to reflect its new return type.

@jan-ivar
Copy link
Member Author

@martinthomson does this LGTY to merge, or should we keep bikeshedding?

@jan-ivar
Copy link
Member Author

With the benfit of some hindsight, I'm questioning whether our concerns over understandability justify the awkward naming:

dictionary WebTransportSendStreamStats {
  DOMHighResTimeStamp timestamp;
  unsigned long long amountWrittenTo;
  unsigned long long sentProgress;
  unsigned long long acknowledgedProgress;
};

dictionary WebTransportReceiveStreamStats {
  DOMHighResTimeStamp timestamp;
  unsigned long long receivedProgress;
  unsigned long long amountReadFrom;
};

The following does seem simpler:

dictionary WebTransportSendStreamStats {
  DOMHighResTimeStamp timestamp;
  unsigned long long bytesWritten;
  unsigned long long bytesSent;
  unsigned long long bytesAcknowledged;
};

dictionary WebTransportReceiveStreamStats {
  DOMHighResTimeStamp timestamp;
  unsigned long long bytesReceived;
  unsigned long long bytesRead;
};

@jan-ivar
Copy link
Member Author

Meeting:

  • Add note to sentProgress to mention this is progress of app data sent on a single stream only and does not include any network overhead.
  • Add a note that acknowledgedProgress will match sentProgress over HTTP/2
  • Reorder received ahead of amountReadFrom
  • Go with the latter naming.

@jan-ivar jan-ivar merged commit 66b0772 into w3c:main Apr 27, 2022
@jan-ivar jan-ivar deleted the perstreamstats branch April 27, 2022 17:17
github-actions bot added a commit that referenced this pull request Apr 27, 2022
SHA: 66b0772
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.

Per-stream stats
4 participants