875097 - Add Telemetry probe for number of webRTC calls

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Comment 1

[Ben Brittain (:bbrittain)]

Attachment: Add Telemetry probe for number of webRTC calls; r=derf

Comment 2

[Ben Brittain (:bbrittain)]

Attachment: Add Telemetry probe for number of webRTC calls; r=derf

Ignore the last one. For some reason it appears to be missing my Histogram.json modification.

Comment 3

[Ben Brittain (:bbrittain)]

Attachment: Telemetry for number of calls a session; r=derf

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

Comment 4

[Timothy B. Terriberry (:derf)]

Comment on attachment 754027 [details] [diff] [review]
Telemetry for number of calls a session; r=derf

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

r- for trying to update telemetry data _after_ it's been saved to the current profile.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +249,5 @@
>    mCCM->removeCCObserver(this);
> +
> +#ifdef MOZILLA_INTERNAL_API
> +  // Report number of connected calls during the browser session
> +  Telemetry::Accumulate(Telemetry::WEBRTC_CALL_COUNT, mConnectionCounter);

I don't think anything after profile-before-change2 actually gets saved?

This should probably be in profile-change-net-teardown.

Also, on Android, we can get killed without warning as soon as we enter the background, so we're very likely to under-report with this approach (see comments in toolkit/components/telemetry/TelemetryPing.js). Not sure there's much we can usefully do about that, but it may be worth commenting on in the code so it isn't forgotten.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.h
@@ +17,5 @@
>  #include "CC_LineInfo.h"
>  #include "CC_Observer.h"
>  #include "CC_FeatureInfo.h"
>  
> +#include "mozilla/Telemetry.h"

This doesn't look like it's used for anything in PeerConnectionCtx.h. Can you just remove it?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1467,5 @@
>    mStartTime = mozilla::TimeStamp::Now();
> +
> +  // Increment session call counter
> +  int newCount = PeerConnectionCtx::GetInstance()->mConnectionCounter + 1;
> +  PeerConnectionCtx::GetInstance()->mConnectionCounter = newCount;

Instead of messing with the local variable, why not just use ++?

Comment 5

[Maire Reavy [:mreavy]]

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

Comment 6

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

Attachment: Telemetry for number of calls per session (2)

Updated with feedback.
- Rebased to follow bug 874670 and bug 874175.
- Refactored to update telemetry data in profile-change-net-teardown
- Added comment about android
- Removed unused include
- Removed local var

I've verified that the call to update telemetry data happens before the telemetry sub-system is uninstalled, but I'm not seeing the data in about:telemetry, which appears to show data from the current session only (is that correct?) - Tips on testing this are welcome.

Try with all three patches - https://tbpl.mozilla.org/?tree=Try&rev=253fa05c0759

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> I've verified that the call to update telemetry data happens before the
> telemetry sub-system is uninstalled, but I'm not seeing the data in
> about:telemetry, which appears to show data from the current session only
> (is that correct?) - Tips on testing this are welcome.

That's correct.  If you want to be able to test this, I think your best bet is to do something similar to how IMAGE_DECODE_COUNT is maintained:

http://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#414
http://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#2041

and record data into the histogram as PeerConnectionImpls are created and destroyed, rather than waiting until we're shutting down the browser to tell telemetry about the data.

(The problem here is really a mismatch between the data you want to collect--a single number--and the only convenient mechanism we have from C++ to provide data to telemetry--a histogram for binned data.  We have open bugs on fixing this.)

Comment 8

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

> If you want to be able to test this, I think your best bet is to do something
> similar to how IMAGE_DECODE_COUNT is maintained:

Makes sense. I'll modify the patch to use that technique instead.

>  "WEBRTC_CALL_COUNT":{
>    "kind": "linear",
>    "high": "1000",
>    "n_buckets": "100",
>    "description": "The number of calls made during a session."

Btw. after looking at IMAGE_DECODE_COUNT, I propose we tune our telemetry parameters to match:

  "WEBRTC_CALL_COUNT":{
    "kind": "exponential",
    "high": "500",
    "n_buckets": 50,
    "description": "The number of calls made during a session."
  },

mostly to use exponential, as it seems reasonable to have more buckets near zero (if you're making more than 500 calls per session, I think we're happy).

I wanted to mention this, since these things are apparently hard to change after the fact. From https://developer.mozilla.org/en-US/docs/Performance/Adding_a_new_Telemetry_probe#Miscellaneous

Comment 9

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

Attachment: Telemetry for number of calls per session (3)

Updated to match feedback and comments.
- Avoids telemetry code on shutdown, using same approach as IMAGE_DECODE_COUNT.
- Changed settings to kind:exponential, high:500, n_buckets:50.

Comment 10

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

Forgot to say I verified that data appears in about:telemetry.

Did a new try since another .h file is involved. - https://tbpl.mozilla.org/?tree=Try&rev=f5ca5abef2a2

Comment 11

[Timothy B. Terriberry (:derf)]

Comment on attachment 796335 [details] [diff] [review]
Telemetry for number of calls per session (3)

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

Looks great.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89e5c3399952

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89e5c3399952