1188376 - Separate Hello from non-Hello calls in Telemetry

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Split Hello Telemetry values from general WebRTC

also fixes a minor issue where very short time deltas caused out-of-range bandwidth to be reported (part of bug 1188441)

Comment 2

[Adam Roach [:abr]]

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 3

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

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..."

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: interdiffs

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Split Hello Telemetry values from general WebRTC

* * *
Bug 1188376: interdiffs

Comment 6

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

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?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d44c5c1195c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d44c5c1195c

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eecbf4d2cfe4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/270eca2204c5