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

WebRTC Stats updates #26513

Closed
wants to merge 5 commits into from
Closed

WebRTC Stats updates #26513

wants to merge 5 commits into from

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented May 1, 2023

EDIT: This will be broken up into other PRS and then closed. Keeping open for short term tracking.

Replacement PRs.

This updates WebRTC Stats to reflect that stats are returned in a RTCStatsReport and are effectively part of that interface, rather than in separate dictionaries.

This is part of #26146 and follows on from BCD updates in mdn/browser-compat-data#18910

@github-actions github-actions bot added the Content:WebAPI Web API docs label May 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

Preview URLs (6 pages)
Flaws (8)

Note! 3 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/API/RTCPeerConnection/getStats
Title: RTCPeerConnection: getStats() method
Flaw count: 1

  • broken_links:
    • Can't resolve /en-US/docs/Web/API/RTCStatsReport/peer_connection_stats

URL: /en-US/docs/Web/API/RTCRtpStreamStats
Title: RTCRtpStreamStats
Flaw count: 4

  • macros:
    • /en-US/docs/Web/API/RTCCodecStats/codec does not exist
    • /en-US/docs/Web/API/RTCTransportStats does not exist
    • /en-US/docs/Web/API/RTCCodecStats does not exist
  • bad_bcd_queries:
    • No BCD data for query: api.RTCRtpStreamStats

URL: /en-US/docs/Web/API/RTCInboundRtpStreamStats
Title: RTCInboundRtpStreamStats
Flaw count: 3

  • macros:
    • /en-US/docs/Web/API/RTCReceivedRtpStreamStats does not exist
    • /en-US/docs/Web/API/RTCReceivedRtpStreamStats does not exist
  • bad_bcd_queries:
    • No BCD data for query: api.RTCInboundRtpStreamStats

(comment last updated: 2023-05-20 00:20:55)

@@ -0,0 +1,68 @@
---
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The BCD for RTCStatsReport shows there are a lot of stats (100+) split across about 13 categories.

I have started by documenting in separate category docs. This makes sense to me because there are so many in each category, and I can see RTCStatsReport being impossible to parse. Also IFF I end up documenting properties on their own, there are duplicate names so I'd want to have a sub folder anyway.

@wbamberg @teoli2003 Does this make sense to you too? Easy to move it back into the stats report.

What "type" would it have for the page-type?

Copy link
Collaborator

@wbamberg wbamberg May 1, 2023

Choose a reason for hiding this comment

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

Sorry if this is ignorant, but why isn't this a RTCPeerConnectionStats dictionary with a page type of web-api-interface?

I know we want to remove dictionaries in cases where they can be documented inline, but since we have a dedicated page for this thing, that doesn't seem to apply.

Copy link
Collaborator Author

@hamishwillee hamishwillee May 1, 2023

Choose a reason for hiding this comment

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

@wbamberg Because we don't document pure dictionaries anymore.

What the end user gets is a promise that resolves to an RTCStatsReport that they can iterate for sets of statistics, one of which is the peer-connection set. The RTCPeerConnectionStat is just part of what is in that returned object. Not such a big deal in this case, but some of them are more complex.

Essentially doing it this way is far easier for an end user to understand what is actually returned in a particular set of statistics.

It doesn't have to be a separate doc. I just suggested that because otherwise the stats doc is huge. But I wouldn't do it as a dictionary. The spec authors are also moving in this direction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wbamberg Because we don't document pure dictionaries anymore.

We do though: https://developer.mozilla.org/en-US/docs/Web/API/AesCtrParams - in this case we actually removed and reinstated it because the docs were better with it (#23170).

My understanding of our policy wrt dictionaries is: we document them inline when it makes the docs better to do so (which is usually the case), but are pragmatic about cases where it is better to document them as separate entities.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, I don't really have an opinion about whether this one should be documented standalone as a dictionary or inline. But if it's getting a dedicated page, then we are documenting it standalone, and if we are doing that, then it is better to use the standard way to do this, which AFAICT is to document it as a dictionary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand your point. I'm also quite certain that as reader I will find docs done this way many many times easier to understand.

For now I'll stick with doing the docs this way. Fallback is to put it all in the RTCStatsReport. Breaking it up into dictionaries again would be my last resort - months to get the data in the current form in BCD.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding of our policy wrt dictionaries is: we document them inline when it makes the docs better to do so (which is usually the case), but are pragmatic about cases where it is better to document them as separate entities.

Isn't the preference the other way around? To only document dictionaries separately if it makes the documents better, but prefer to document them inline?

Having been a part of researching RTC stats, I can definitely say that documenting the stats dictionaries separately is NOT better. Each type of RTC stat may have properties from many different dictionaries (it's not a 1:1 comparison). In fact, I actually turned to the spec to decipher the stats types and dictionaries after attempting to sift through the current MDN pages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Me too. Though now I understand it I could probably create a slightly better dictionary based-document than MDN has now.

@wbamberg I'm hoping we accept this is better and if so, we will need to think about a page type. Right now this puts me in a difficult - where do I go from here position.

- : A floating-point value indicating the average {{Glossary("RTCP")}} interval between two consecutive compound RTCP packets.
- {{domxref("RTCInboundRtpStreamStats.bytesReceived", "bytesReceived")}}
- : A 64-bit integer which indicates the total number of bytes that have been received so far for this media source.
<!-- RTCInboundRtpStreamStats -->
Copy link
Collaborator Author

@hamishwillee hamishwillee May 1, 2023

Choose a reason for hiding this comment

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

FYI since at this point I don't know if I'll be forced to revert to dictionaries or some other approach, am doing both.

  • organises to match the spec order, and the inbound-rtp doc.
  • commented out items that are in spec but not implemented according to testing
  • Deleted items that are not in current spec or BCD testing. Appreciate that this is information loss, but most of that churn happened 4 or 5 years ago - this is no worse and significantly better. Still need to delete the sub-docs
    • packetsDuplicated, packetsFailedDecryption, RTCInboundRtpStreamStats.perDscpPacketsReceived, RTCInboundRtpStreamStats.receiverId, RTCInboundRtpStreamStats.sliCount, RTCInboundRtpStreamStats.trackId, RTCInboundRtpStreamStats.trackId

@@ -0,0 +1,168 @@
---
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just the things that are implemented with placeholders. Will copy stuff across, if needed, when how we are managing this is agreed.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jun 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@hamishwillee
Copy link
Collaborator Author

Closing this. Superseded by a number of other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants