Closed Bug 874670 Opened 7 years ago Closed 6 years ago

add Telemetry data for webRTC call duration

Categories

(Core :: WebRTC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bbrittain, Assigned: jib)

References

(Blocks 1 open bug)

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(1 file, 5 obsolete files)

No description provided.
Assignee: nobody → ben
Whiteboard: [WebRTC][blocking-webrtc-]
Blocks: 874175
No longer blocks: 874175
Attached patch Telemetry for ICE call duration (obsolete) — Splinter Review
Attachment #752960 - Flags: review?(tterribe)
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?
Attachment #752960 - Flags: review?(tterribe) → review-
Attachment #752960 - Attachment is obsolete: true
Attachment #753036 - Flags: review?(tterribe)
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.
Attachment #753036 - Flags: review?(tterribe) → review-
Attachment #753036 - Attachment is obsolete: true
Attachment #753064 - Flags: review?(tterribe)
Attachment #753064 - Attachment is obsolete: true
Attachment #753064 - Flags: review?(tterribe)
Attachment #753347 - Flags: review?(tterribe)
https://tbpl.mozilla.org/?tree=Try&rev=5a733776c0f7
Attachment #753347 - Attachment is obsolete: true
Attachment #753347 - Flags: review?(tterribe)
Attachment #754025 - Flags: review?(tterribe)
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).
Attachment #754025 - Flags: review?(tterribe) → review+
Reassigning to Jan-Ivar since Ben has gone back to school
Assignee: ben → jib
Target Milestone: --- → mozilla26
Rebased to follow bug 874175, and verified that call times appear in about:telemetry. Carrying forward r+ from derf.
Attachment #754025 - Attachment is obsolete: true
Attachment #795582 - Flags: review+
Depends on: 874175
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/bf28665c76ba
Keywords: checkin-needed
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bf28665c76ba
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC][blocking-webrtc-][fixed-in-fx-team] → [WebRTC][blocking-webrtc-]
You need to log in before you can comment on or make changes to this bug.