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 stat for RTTs between client and STUN/TURN server #339

Closed
taylor-b opened this issue Mar 7, 2018 · 9 comments
Closed

Add stat for RTTs between client and STUN/TURN server #339

taylor-b opened this issue Mar 7, 2018 · 9 comments
Assignees

Comments

@taylor-b
Copy link
Contributor

taylor-b commented Mar 7, 2018

We already have totalRoundTripTime in RTCIceCandidatePairStats, and roundTripTime in RTCRemoteInboundRtpStreamStats, but these are both an end-to-end round trip time.

I suggest adding a similar stat to RTCIceCandidateStats, which would accumulate round trip times as measured by STUN transactions between client and STUN server (from initial candidate gathering, keepalives for NAT bindings, TURN refreshes, etc.). We've encountered a couple uses for this:

  • It can provide some information about the endpoint's Internet connection even before connecting to a remote endpoint.
  • Once connected, it can help indicate which side of the connection is contributing more to latency.

So I'd suggest adding something like:

  • totalStunRoundTripTime
  • totalStunResponsesReceived (divide by this to get average RTT)
  • totalStunRequestsSent (allows you to detect packet loss)

Or maybe totalStunServerRoundTripTime would be a better name, to make it extra clear that this is between the client and STUN server, and doesn't include connectivity checks.

@fippo
Copy link
Contributor

fippo commented Mar 7, 2018

It can provide some information about the endpoint's Internet connection even before connecting to a remote endpoint.

One can actually get some data on this by looking at the stun and turn candidate gathering time. But having more than two data points (for udp; only one for tcp) is better.

@alvestrand
Copy link
Contributor

Should this somehow be reported per STUN server?
If we have two STUN servers configured, it seems wrong somehow to report the average number. But if we need that level of complexity, we should introduce an RTCStunServerStats object.

@fippo
Copy link
Contributor

fippo commented Mar 7, 2018

good point... this needs to live on local-candidate then? We might drop the "Stun" in the names then since we have no other RTT.

@alvestrand
Copy link
Contributor

Not sure localCandidate is the right place. One STUN server can end up giving you > 1 candidate, I think.
(or can it?)

@fippo
Copy link
Contributor

fippo commented Mar 7, 2018

hrm... for relay candidates the candidate ip should make the localCandidate unique.

For STUN you're correct... if you ask two different stun servers you might get the same srflx address from both (depending on the nat type; see this which is in vain).
I would hope that the browser actually drops the one with the higher (initial) latency?

@taylor-b
Copy link
Contributor Author

taylor-b commented Mar 7, 2018

My idea was for these fields to go on RTCIceCandidateStats, only for "local-candidate"s.

I hadn't considered the case of multiple STUN servers producing the same server reflexive address. Though this wouldn't be a new problem. RTCIceCandidateStats has a url field, which is "The URL of the TURN or STUN server that translated this IP address". So it assumes every candidate is associated with at most one server. The "candidate gathered" event has a URL field like this too.

So, the spec seems to assume that the implementation will choose one STUN server for each srflx candidate. I know Chrome will just go with the first one that responds.

@alvestrand
Copy link
Contributor

alvestrand commented Mar 14, 2018

I think we have multiple mixups here:

  • One stun server providing multiple addresses
  • multiple stun servers providing one address

Suggest adding a stunserver object, containing:

  • URL
  • local address used to communicate with the server (hostport, including address family)
  • no of requests sent
  • no of responses received
  • sum of RTT for received responses

That would make it independent of candidates.

@alvestrand alvestrand self-assigned this Mar 14, 2018
@taylor-b
Copy link
Contributor Author

One stun server providing multiple addresses

Just to clarify, do you mean a server providing multiple "srflx" addresses for multiple "host" addresses (1:1)? Or for a single "host" address (1:M)? The latter shouldn't happen.

Suggest adding a stunserver object

If I understand correctly, this is really more of "stunserverconnection", since you could have multiple objects with the same URL but different local addresses (for different network interfaces, or different m= sections if bundling isn't negotiated).

Anyway, that should work; probably cleaner than trying to shove things in RTCIceCandidateStats that don't really belong.

@alvestrand alvestrand assigned vr000m and unassigned alvestrand Apr 18, 2018
@vr000m
Copy link
Contributor

vr000m commented May 30, 2018

@lennart-csio PTAL

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

No branches or pull requests

5 participants