Closed Bug 1336182 Opened 7 years ago Closed 7 years ago

Need telemetry for DTLS handshake time

Categories

(Core :: WebRTC: Networking, defect, P2)

Product:
Core
Component:
WebRTC: Networking
Version:
48 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(2 files)

Bug 1336182 - Part 1: Add DTLS handshake time telemetry.
59 bytes, text/x-review-board-request
drno
: review+
benjamin
: review+
Details
Bug 1336182 - Part 2: Use "override" keyword to silence some warnings about inconsistency.
59 bytes, text/x-review-board-request
drno
: review+
Details 
      No description provided.
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
Comment on attachment 8834189 [details]
Bug 1336182 - Part 2: Use "override" keyword to silence some warnings about inconsistency.

https://reviewboard.mozilla.org/r/110218/#review111664
Attachment #8834189 - Flags: review?(drno) → review+
Comment on attachment 8834188 [details]
Bug 1336182 - Part 1: Add DTLS handshake time telemetry.

https://reviewboard.mozilla.org/r/110216/#review111666

Do we have a probe which tells us the over all connection time, so we know how much of the overall time we spend on DTLS?

::: media/mtransport/transportlayerdtls.cpp:1052
(Diff revision 2)
> +  if (state == TS_CONNECTING) {
> +    handshake_started_ = TimeStamp::Now();

Even though it should not happen how about setting |handshake_started_| only if it not set already?

::: media/mtransport/transportlayerdtls.cpp:1289
(Diff revision 2)
> +    case TransportLayer::State::TS_OPEN:
> +      if (role == TransportLayerDtls::CLIENT) {
> +        Telemetry::Accumulate(Telemetry::WEBRTC_DTLS_CLIENT_SUCCESS_TIME, delta);
> +      } else {
> +        Telemetry::Accumulate(Telemetry::WEBRTC_DTLS_SERVER_SUCCESS_TIME, delta);
> +      }

Might be worth checking with mjf or Telemetry folks if it would be better to use the new fancy Scalars for this instead of the long list of single probes.

::: toolkit/components/telemetry/Histograms.json:6927
(Diff revision 2)
> +  "WEBRTC_DTLS_CLIENT_SUCCESS_TIME": {
> +    "alert_emails": ["webrtc-dtls-telemetry-alerts@mozilla.com"],
> +    "bug_numbers": [1336182],
> +    "expires_in_version": "58",
> +    "kind": "exponential",
> +    "high": 10000,
> +    "n_buckets": 20,
> +    "description": "The length of time (in milliseconds) it took for a client DTLS handshake to complete, given that DTLS succeeded."
> +  },

Do we need review from a Telemetry person for this?
Attachment #8834188 - Flags: review?(drno) → review+
Comment on attachment 8834188 [details]
Bug 1336182 - Part 1: Add DTLS handshake time telemetry.

https://reviewboard.mozilla.org/r/110216/#review111666

Not yet.

> Even though it should not happen how about setting |handshake_started_| only if it not set already?

I can add a state check I guess.

> Might be worth checking with mjf or Telemetry folks if it would be better to use the new fancy Scalars for this instead of the long list of single probes.

Nah, we want the whole sample distribution here.

> Do we need review from a Telemetry person for this?

I bet we do.
Comment on attachment 8834188 [details]
Bug 1336182 - Part 1: Add DTLS handshake time telemetry.

https://reviewboard.mozilla.org/r/110216/#review111948

What exactly do you want review on? That would be helpful to include in the future.
I just took a quick look at the histogram usage here, which seems fine overall.

You still need data collection review for any new Telemetry collection:
https://wiki.mozilla.org/Firefox/Data_Collection

::: media/mtransport/transportlayerdtls.cpp:1332
(Diff revision 3)
> +    TransportLayer::State endState) {
> +  TimeDuration delta = TimeStamp::Now() - handshake_started_;
> +  nsCOMPtr<nsIThread> mainThread;
> +  NS_GetMainThread(getter_AddRefs(mainThread));
> +  RUN_ON_THREAD(mainThread,
> +                WrapRunnableNM(RecordHandshakeCompletionTelemetry_m,

Use `NS_DispatchToMainThread()`?
But recording into Telemetry is thread-safe, so you can just accumulate on the same thread.
Attachment #8834188 - Flags: review?(gfritzsche)
Comment on attachment 8834188 [details]
Bug 1336182 - Part 1: Add DTLS handshake time telemetry.

https://reviewboard.mozilla.org/r/110216/#review111948

Ah, the data collection review is separate from telemetry itself. I see.

> Use `NS_DispatchToMainThread()`?
> But recording into Telemetry is thread-safe, so you can just accumulate on the same thread.

It is? Has that always been the case? Because I think we've run into problems with that in the past...
(In reply to Byron Campen [:bwc] from comment #11)
> > Use `NS_DispatchToMainThread()`?
> > But recording into Telemetry is thread-safe, so you can just accumulate on the same thread.
> 
> It is? Has that always been the case? Because I think we've run into
> problems with that in the past...

It was made thread-safe in the last year, so that is a bit newer.
Comment on attachment 8834188 [details]
Bug 1336182 - Part 1: Add DTLS handshake time telemetry.

https://reviewboard.mozilla.org/r/110216/#review112966

data-review=me (I did not review any code)
Attachment #8834188 - Flags: review?(benjamin) → review+
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98b4c49e8a38
Part 1: Add DTLS handshake time telemetry. r=bsmedberg,drno
https://hg.mozilla.org/integration/autoland/rev/8b899017b08a
Part 2: Use "override" keyword to silence some warnings about inconsistency. r=drno
https://hg.mozilla.org/mozilla-central/rev/98b4c49e8a38
https://hg.mozilla.org/mozilla-central/rev/8b899017b08a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: