Closed Bug 1188376 Opened 5 years ago Closed 5 years ago

Separate Hello from non-Hello calls in Telemetry

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed
Blocking Flags:

People

(Reporter: jesup, Assigned: jesup)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Since Hello is much more controlled (especially in pre-release versions, but also release), we should get 'cleaner' stats unaffected by people trying services that may not have TURN servers, people running automated tests, etc.
Depends on: 1189099
Assignee: nobody → rjesup
backlog: --- → webRTC+
Rank: 10
Priority: -- → P1
also fixes a minor issue where very short time deltas caused out-of-range bandwidth to be reported (part of bug 1188441)
Attachment #8645810 - Flags: review?(jib)
Comment on attachment 8645810 [details] [diff] [review]
Split Hello Telemetry values from general WebRTC

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

I didn't do a full review, but wanted to make an important comment on how you detect Hello-related pages.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +694,5 @@
>      location->Release();
>  
>      CopyUTF16toUTF8(locationAStr, locationCStr);
> +#define HELLO_URL_START "https://hello.firefox.com/"
> +    mIsHello = (strncmp(HELLO_URL_START, locationCStr.get(), strlen(HELLO_URL_START)) == 0);

This is only going to catch half of the metrics you want to assign to Hello. The built-in client will be on URLs that start with "about:loopconversation" -- although the most future-proof thing would probably be to simply check for URLs that start with "about:loop"
Comment on attachment 8645810 [details] [diff] [review]
Split Hello Telemetry values from general WebRTC

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +240,5 @@
>            MOZ_ASSERT(s.mIsRemote);
> +          ID id;
> +          if (isAudio) {
> +            id = isHello ? WEBRTC_AUDIO_QUALITY_OUTBOUND_RTT :
> +                           LOOP_AUDIO_QUALITY_OUTBOUND_RTT;

These are backwards (isNotHello), as are all the others in this file.

@@ +282,3 @@
>                }
> +              // XXX accumulate values until enough time has passed and then Accumulate()
> +              // Not critical

?? Add bug number and a little more explanation, or remove.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +745,5 @@
>    std::string mHandle;
>  
>    // A name for this PC that we are willing to expose to content.
>    std::string mName;
> +  bool mIsHello; // For telemetry; doesn't have to be 100% right

Not sure what this comment is trying to say. Are there cases where it is wrong?

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +825,5 @@
>  
>  static void StoreLongTermICEStatisticsImpl_m(
>      nsresult result,
> +    nsAutoPtr<RTCStatsQuery> query,
> +    bool aIsHello) {

Most other places in the code I think we call it "Loop", not "Hello".

::: toolkit/components/telemetry/Histograms.json
@@ +8671,5 @@
>      "expires_in_version": "44",
>      "kind": "boolean",
>      "description": "Was there an error while performing the v7 permissions DB migration?"
> +  },
> +  "LOOP_ICE_FINAL_CONNECTION_STATE": {

Patch seems to use Hello and loop interchangeably. We should perhaps be consistent, but I'm not sure with what.

Should we call it HELLO_ here since these are seen in the telemetry UI?

@@ +8706,5 @@
> +  "LOOP_CANDIDATE_TYPES_GIVEN_FAILURE": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 128,
> +    "description": "Identical to WEBRTC_CANDIDATE_TYPES_GIVEN_SUCCEESS, except recorded only when ICE fails on the stream in question."

"Identical to LOOP_CANDIDATE_TYPES_GIVEN_SUCCESS..."
Attachment #8645810 - Flags: review?(jib)
Attachment #8645810 - Attachment is obsolete: true
Comment on attachment 8645989 [details] [diff] [review]
interdiffs

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

I still think mIsLoop would be more consistent, but no biggie.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +694,5 @@
>      location->Release();
>  
>      CopyUTF16toUTF8(locationAStr, locationCStr);
> +#define HELLO_CLICKER_URL_START "https://hello.firefox.com/"
> +#define HELLO_INITIATOR_URL_START "about:loopconversation"

Not "about:loop" like Adam suggested in comment 2?

@@ +3401,3 @@
>    int &cnt = PeerConnectionCtx::GetInstance()->mConnectionCounter;
> +  if (cnt > 0) {
> +    Telemetry::GetHistogramById(Telemetry::WEBRTC_CALL_COUNT)->Subtract(cnt);

Any harm in subtracting zero?
Attachment #8645989 - Flags: review+
Attachment #8645990 - Flags: review?(jib) → review+
(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)

> Not "about:loop" like Adam suggested in comment 2?

Yeah, I can shorten it to that.

> 
> @@ +3401,3 @@
> >    int &cnt = PeerConnectionCtx::GetInstance()->mConnectionCounter;
> > +  if (cnt > 0) {
> > +    Telemetry::GetHistogramById(Telemetry::WEBRTC_CALL_COUNT)->Subtract(cnt);
> 
> Any harm in subtracting zero?

It also keeps a count of things added, and asserts on it being negative.  I ran across this when I tried to have them separate, where the count would go up, but would get put into two different buckets.  It's just safer like this.
https://hg.mozilla.org/mozilla-central/rev/5d44c5c1195c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8645990 [details] [diff] [review]
Split Hello Telemetry values from general WebRTC

Approval Request Comment
[Feature/regressing bug #]: n/a

[User impact if declined]: Longer wait until we can see useful telemetry results with a controlled environment (we plan to use Hello/Loop telemetry values to verify if results seen across all uses are valid or not, and to look into certain cases).  Also fixes a problem with how bitrates are reported.


[Describe test coverage new/current, TreeHerder]: N/A

[Risks and why]: Exceedingly low risk

[String/UUID change made/needed]: none
Attachment #8645990 - Flags: approval-mozilla-aurora?
Attachment #8645990 - Flags: approval-mozilla-beta?
Comment on attachment 8645990 [details] [diff] [review]
Split Hello Telemetry values from general WebRTC

Early in the beta cycle, will help the team, taking it.
Attachment #8645990 - Flags: approval-mozilla-beta?
Attachment #8645990 - Flags: approval-mozilla-beta+
Attachment #8645990 - Flags: approval-mozilla-aurora?
Attachment #8645990 - Flags: approval-mozilla-aurora+
Blocks: 1198345
You need to log in before you can comment on or make changes to this bug.