WIP: Add Stats structs #611
Conversation
maxhawkins
force-pushed
the
getstats
branch
6 times, most recently
from
e8adb27
to
b29d7e3
Compare
|
@Sean-Der Do you think it would be useful or confusing to merge these struct definitions? I don't have time to implement the stats just yet, but this will make it easier for someone to do it in the future. Alternately I could hang onto this PR until I have time to add a stat. |
|
IMO I would merge them! someone might get interested and finish the work. Would you mind adding some dumb tests that just assert things exist? just so coverage doesn't drop |
|
Will do |
maxhawkins
force-pushed
the
getstats
branch
4 times, most recently
from
7f65eac
to
aad5dd3
Compare
|
PTAL |
Definitions from https://www.w3.org/TR/webrtc/#dom-rtcstats Related to #610
Related to #610
| // Report (RR) or Extended Report (XR). | ||
| type ReceivedRTPStreamStats struct { | ||
| // PacketsReceived is the total number of RTP packets received for this SSRC. | ||
| PacketsReceived uint32 `json:"packetsReceived"` |
There was a problem hiding this comment.
NIT -- What do you think of putting a newline after every member? Visually I break chunks apart by newline, so easier to consume when reading
stats.go
Outdated
| // StatsReport collects Stats objects indexed by their ID. | ||
| type StatsReport map[string]Stats | ||
|
|
||
| // StatsBase contains all the fields Stats objects share |
There was a problem hiding this comment.
On second thought, let's just delete it.
If I get rid of the Stats methods like I mention below all of these fields can just be duplicated in the base stat objects.
stats.go
Outdated
|
|
||
| // A Stats object contains a set of statistics copies out of a monitored component | ||
| // of the WebRTC stack at a specific time. | ||
| type Stats interface { |
There was a problem hiding this comment.
Not a big deal, but we are putting some implementation details in the users face by having these public.
I don't feel strongly, but would be nice if godoc doesn't show anything we did to make our code shorter
There was a problem hiding this comment.
I do want to keep the type around. Maybe i'll get rid of the methods:
type Stats interface{}I'm not sure Type() and Timestamp() are so useful anyway. Everyone will end up casting to the base type and they can find those values there.
The initial idea for this had to do with keeping the results of the not-yet-implemented GetStats method typesafe. It returns a map with polymorphic values map[string]Stats.
There was a problem hiding this comment.
Damn really nice work @maxhawkins
This is going to be great in the godoc, getting info on stats for WebRTC right now is awful. You have to dig into things at runtime/look at MDN. Really excited to see this go out
|
Is this done？exciting |
Add structs from rtc-domstats spec so we can begin implementing the GetStats API.
Related to #610