874670 - add Telemetry data for webRTC call duration

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Comment 1

[Ben Brittain (:bbrittain)]

Attachment: Telemetry for ICE call duration

Comment 2

[Timothy B. Terriberry (:derf)]

Comment on attachment 752960 [details] [diff] [review]
Telemetry for ICE call duration

Review of attachment 752960 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +592,5 @@
> +#endif
> +
> +#ifdef MOZILLA_INTERNAL_API
> +  // Telemetry for call start
> +  mStartTime = mozilla::TimeStamp::Now(); 

I don't think this is the right place to do this. This starts counting the call duration as soon as the PC is created. A page may create a PC as soon as the page loads so that it can start gathering ICE candidates, long before it knows the user wants to begin a call. It may throw that PC away without ever starting a call.

Also, trailing whitespace.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +353,5 @@
>  #endif
>  
>    nsRefPtr<PeerConnectionMedia> mMedia;
>  
> +  // Start time used for telemetry

Start of what?

Comment 3

[Ben Brittain (:bbrittain)]

Attachment: add Telemetry data for webRTC call duration

Comment 4

[Timothy B. Terriberry (:derf)]

Comment on attachment 753036 [details] [diff] [review]
add Telemetry data for webRTC call duration

Review of attachment 753036 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +235,5 @@
>              mObserver->OnAddStream(stream);
>            }
> +
> +#ifdef MOZILLA_INTERNAL_API
> +          mPC->setStartTime();

So if you do this here, you'll only set a start time when the remote side has added at least one stream, which might not always happen. You'll also start counting the clock as soon as the remote description gets set (plus the latency for SIPCC to process that remote description and generate theis event, but that should be small).

If you want to include this time in the call, you should probably do this in SETREMOTEDESC instead of here. If you want to start when the media actually starts flowing, you should do this the first time ICE completes successfully.

You probably want to make sure this only gets executed once per PeerConnection.

@@ +1246,5 @@
>      return;
>  
> +#ifdef MOZILLA_INTERNAL_API
> +  // End of call to be recorded in Telemetry
> +  mozilla::TimeDuration timeDelta = mozilla::TimeStamp::Now() - mStartTime;

It's now possible for you to destroy a PeerConnection which has never had its mStartTime set (because a call was never started). You probably don't want to submit a call duration value for such PCs.

@@ +1454,5 @@
>  
> +//Telemetry set start time
> +void
> +PeerConnectionImpl::setStartTime() {
> +  mStartTime = mozilla::TimeStamp::Now(); 

Trailing whitespace.

::: toolkit/components/telemetry/Histograms.json
@@ +3311,5 @@
>      "high": "10000",
>      "n_buckets": "1000",
>      "description": "The time (in milliseconds) that it took a 'detach' request to go round trip."
> +  },
> +  "WEBRTC_CALL_DURATION":{

Might want to base this on top of bug 874175 or someone will have conflicts when trying to merge.

Comment 5

[Ben Brittain (:bbrittain)]

Attachment: add Telemetry data for webRTC call duration

Comment 6

[Ben Brittain (:bbrittain)]

Attachment: Telemetry for call duration; r=derf

Comment 7

[Ben Brittain (:bbrittain)]

Attachment: add Telemetry data for webRTC call duration; r=derf

https://tbpl.mozilla.org/?tree=Try&rev=5a733776c0f7

Comment 8

[Timothy B. Terriberry (:derf)]

Comment on attachment 754025 [details] [diff] [review]
add Telemetry data for webRTC call duration; r=derf

Review of attachment 754025 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a couple of nits.
Again, don't forget to update the commit message.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1458,5 @@
>    return mSDPParseErrorMessages;
>  }
>  
> +#ifdef MOZILLA_INTERNAL_API
> +//Telemetry set start time

Please make this into a coherent sentence.

@@ +1461,5 @@
> +#ifdef MOZILLA_INTERNAL_API
> +//Telemetry set start time
> +void
> +PeerConnectionImpl::setStartTime() {
> +  mStartTime = mozilla::TimeStamp::Now();

I think you should check to make sure mStartTime is only set once. In the future when we support re-negotiation, we may successfully set the remote description multiple times.

::: toolkit/components/telemetry/Histograms.json
@@ +3319,5 @@
> +  "WEBRTC_CALL_DURATION":{
> +    "kind": "exponential",
> +    "high": "10000",
> +    "n_buckets": "1000",
> +    "description": "The length of time (in seconds) that a call lasted."

I don't know if it's worth documenting that this gives us 5 minutes worth of 1-second resolution before the bins start getting larger than 1 second (which seems reasonable to me).

Comment 9

[Maire Reavy [:mreavy]]

Reassigning to Jan-Ivar since Ben has gone back to school

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Telemetry for call duration. r=derf

Rebased to follow bug 874175, and verified that call times appear in about:telemetry. Carrying forward r+ from derf.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/bf28665c76ba

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf28665c76ba