Closed Bug 875097 Opened 7 years ago Closed 6 years ago

Add Telemetry probe for number of webRTC calls

Categories

(Core :: WebRTC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bbrittain, Assigned: jib)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 4 obsolete files)

No description provided.
Whiteboard: [WebRTC],[blocking-webrtc-]
Assignee: nobody → ben
Attachment #753498 - Flags: review?(tterribe)
Ignore the last one. For some reason it appears to be missing my Histogram.json modification.
Attachment #753498 - Attachment is obsolete: true
Attachment #753498 - Flags: review?(tterribe)
Attachment #753538 - Flags: review?(tterribe)
https://tbpl.mozilla.org/?tree=Try&rev=5a733776c0f7
Attachment #753538 - Attachment is obsolete: true
Attachment #753538 - Flags: review?(tterribe)
Attachment #754027 - Flags: review?(tterribe)
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 ++?
Attachment #754027 - Flags: review?(tterribe) → review-
Reassigning to Jan-Ivar since Ben has gone back to school
Assignee: ben → jib
Target Milestone: --- → mozilla27
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
Attachment #754027 - Attachment is obsolete: true
Attachment #795685 - Flags: review?(tterribe)
(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.)
> 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
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.
Attachment #795685 - Attachment is obsolete: true
Attachment #795685 - Flags: review?(tterribe)
Attachment #796335 - Flags: review?(tterribe)
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 on attachment 796335 [details] [diff] [review]
Telemetry for number of calls per session (3)

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

Looks great.
Attachment #796335 - Flags: review?(tterribe) → review+
https://hg.mozilla.org/mozilla-central/rev/89e5c3399952
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.