Add counters for bytes sent by ICE pings (#534)

[jonasore]

• Fixes Add metric for bytes sent for connectivity/consent requests and responses #534

[henbos]

@alvestrand can you take a look? Is connectivity bytes for checks well-defined?

[vr000m]

Does this include the headers or only the payload? We should be consistent across the dictionaries that the measurement of bytesSent and bytesReceived.

[alvestrand]

The definition of "bytesSent" should be clear about whether ICE checks are counted or not. At the moment, I think it's not clear.
(I think they should not be included in "bytesSent", because they are overhead seen from the user. But the most important thing is that it is clear.)

[henbos]

RTCIceCandidatePairStats.bytesSent is said to only be payload bytes. Does ICE pings contain any payload?

How do you intend to use these counters? I'm wondering if the current implementation of candidate pair bytes sent is correct.

Or if you want to subtract ICE pings from the transport bytes sent I wonder if we need to add these metrics to the transport dictionary.

Do we know ICE candidate pair stats are supposed to be deleted?

[henbos]

Can you add text to bytesSent to say whether STUN pings are included or not?

[jonasore]

Answers:

1. Does this include the headers or only the payload? We should be consistent across the dictionaries that the measurement of bytesSent and bytesReceived.

This payload for the ICE pings. I'm not sure about ip headers as such, but I assume they are not counted. (can check in rest of documentation about that)

2. RTCIceCandidatePairStats.bytesSent is said to only be payload bytes. Does ICE pings contain any payload?

No

3. How do you intend to use these counters? I'm wondering if the current implementation of candidate pair bytes sent is correct.

I modified an ICE agent to send more pings, I would like to be able to add up how much that "cost" and compare it to the actual payload sent. I'm currently not 100% certain to how they are counted wrt to the existing counters.

4. Do we know ICE candidate pair stats are supposed to be deleted?

Don't understand.

[henbos]

When we talk about header bytes in other places we are not talking about transport-layer headers like TCP or UDP. So for RTP packets, the headers are the size of the RTP headers.

The ICE packets should contain an ICE protocol header, to distinguish it from RTP packets, I guess?
Do we need a ICE headerBytesSent or is this not of interest?

2. RTCIceCandidatePairStats.bytesSent is said to only be payload bytes. Does ICE pings contain any payload?

No

So if I sent 1 kB of ICE ping I would expect bytesSent to increase by 0?
Then what makes up an ICE ping packet used for probing. Is the ICE header massive? Or are we sending lots and lots of tiny ICE pings but they add up to a lot in the end?

[jonasore]

1. I don't think it adds value to talk about ICE protocol header...just bytes,
   i.e the metric should include all bytes sent as STUN pings.

2. "ICE ping packed used for probing", parse error.
   The RTP probing is done using RTP packets.

3. "Or are we sending lots and lots of tiny ICE pings but they add up to a lot in the end?"
   Sort of yes, the ICE agent that I'm working on constraints ICE pings to X bits/sec.
   Bitrate for RTP is sufficiently small, then X is noticable.

4. I added this cause I've changed the frequency of the ICE pings,
   and there was no real way to measure that.

[jonasore]

that said...i'm no longer super worried about my extra bytes...
(but I still didn't find a way to measure)

[alvestrand]

I think it would make sense to treat bytes sent in ICE packets as "overhead only" - that is, not count them in RTCIceCandidatePair.bytesSent. But if they're currently counted as part of bytesSent, it's a compatibility problem to change that, and we should change the documentation of bytesSent instead.

Jonas, do you know if they're counted at present or not?

[henbos]

One question is if they are included in bytesSent or not in the current chromium implementation.
Another question is what the pings look like on the wires. So for example, if they are a special type of payload that the sender knows about but not the receiver, this could cause the receiver endpoints' bytesReceived to increase. Or not if pings are really payload-neutral...

[jonasore]

first looking at send: it seems that the bytes are not included in bytesSent
a) sending on ping: https://source.chromium.org/chromium/chromium/src/+/master:third_party/webrtc/p2p/base/connection.cc;l=835;drc=34ad93361f96ad875bf350ced27e15b7aa126113?originalUrl=https:%2F%2Fcs.chromium.org%2F
b) sending of RTP:
https://source.chromium.org/chromium/chromium/src/+/master:third_party/webrtc/p2p/base/connection.cc;l=1353;drc=34ad93361f96ad875bf350ced27e15b7aa126113?originalUrl=https:%2F%2Fcs.chromium.org%2F

The send_tracker in then used to populate stats object: https://source.chromium.org/chromium/chromium/src/+/master:third_party/webrtc/p2p/base/connection.cc;l=1206;drc=34ad93361f96ad875bf350ced27e15b7aa126113?originalUrl=https:%2F%2Fcs.chromium.org%2F

===

This would mean that we could add them as "overhead" right ?

[jonasore]

similar for recv:
https://source.chromium.org/chromium/chromium/src/+/master:third_party/webrtc/p2p/base/connection.cc;l=457;drc=34ad93361f96ad875bf350ced27e15b7aa126113?originalUrl=https:%2F%2Fcs.chromium.org%2F

only non-STUN is added to bytesRecevied.

[jonasore]

henbos question: all ICE messages should be STUN messages,
so I think this is safe atleast as coded today...

[alvestrand]

Seems we're clear then!
Merging this one, and filing an issue about clarifying that bytesSent does not include ICE bytes.