907755 - add telemetry probe for how long it takes us to display the source text to the user

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Should start timing when the user selects an unfetched source and shouldn't report the probe until right after the source text is displayed.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This would be a good bug for someone who has already completed one or two other debugger bugs.

Comment 2

[Hubert B Manilla]

Hi nick,
Would like to work on this.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sweet :)

So we need a new telemetry probe. In the probe we should record the milliseconds frin when we first select a source in the debugger view, to right after the source is actually displayed to the user.

https://developer.mozilla.org/en-US/docs/Performance/Adding_a_new_Telemetry_probe

This isn't something we can really test super well, so just make sure that a) you can see results for the new probe in about:telemetry after you build and test the debugger on random web pages, and b) all the existing debugger tests pass.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Search for DEVTOOLS_DEBUGGER_RDP for example probe definitions. I think we should actually define two probes here: one for local transports, another for remote transports.

Comment 5

[Hubert B Manilla]

ok..thanks. on it

Comment 6

[Hubert B Manilla]

i found that DEVTOOLS_DEBUGGER_RDP_LOCAL_SOURCES_MS and DEVTOOLS_DEBUGGER_RDP_REMOTE_SOURCES_MS have already being defined in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json 
I'm not sure if these can be used for the probes or should new ones be defined?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Hubert B Manilla from comment #6)
> i found that DEVTOOLS_DEBUGGER_RDP_LOCAL_SOURCES_MS and
> DEVTOOLS_DEBUGGER_RDP_REMOTE_SOURCES_MS have already being defined in
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> Histograms.json 
> I'm not sure if these can be used for the probes or should new ones be
> defined?

Do not reuse these, they are for the request that gets a list of sources from the debugger rather than how long it takes to display a specific source to the user.

I suggest adding something like DEVTOOLS_DEBUGGER_DISPLAY_SOURCE_{REMOTE,LOCAL}.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Er, I mean DEVTOOLS_DEBUGGER_DISPLAY_SOURCE_{REMOTE,LOCAL}_MS. It should end in _MS because we are measuring milliseconds and this is an existing convention.

Comment 9

[Hubert B Manilla]

Hi guys,
I'm having an issue, ive defined the probe in the Histogrm.json file but it does not seem to find the histogramId when i want to add the probe in a file. Am i missing something.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Can you upload your patch and the error + stack trace? Hard to help without much info.

Off the top of my head, you might need to do a full

  $ ./mach build

after adding the telemetry probe. See if you still get the error after that.

Comment 11

[Hubert B Manilla]

ok. i thought of that. but i tried to build just the toolkit/components/telemetry folder but didn't work.
Am running a full build now. will let you know. 
Thanks

Comment 12

[Hubert B Manilla]

it worked, it needed a full build. thanks.

Comment 13

[Hubert B Manilla]

Attachment: Bug 907755 - added telemetry probes for how long it takes us to display the source text to the user

Defined probes in Histogram.json, added probe the debugger-view.js
Not sure how test for LOCAL and REMOTE connections to decide which probe to use

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 804932 [details] [diff] [review]
Bug 907755 - added telemetry probes for how long it takes us to display the source text to the user

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

|DebuggerClient| instances have a |localTransport| boolean property that is true when (drum roll please) the transport is local.

  let transportType = DebuggerController.client.localTransport
    ? "LOCAL"
    : "REMOTE";

::: browser/devtools/debugger/debugger-view.js
@@ +257,5 @@
> +    let histogramId = "DEVTOOLS_DEBUGGER_DISPLAY_SOURCE" + transportType + "_MS";
> +    try {
> +      histogram = Services.telemetry.getHistogramById(histogramId);
> +      startTime = +new Date();
> +    } catch(e){

Small style nit: please add a space between the closing paren and opening curly, like the rest of the file.

I guess this actually doesn't matter since you will remove the try/catch before we land this, right?

@@ +287,5 @@
>        // Synchronize any other components with the currently displayed source.
>        DebuggerView.Sources.selectedValue = aSource.url;
>        DebuggerController.Breakpoints.updateEditorBreakpoints();
>  
> +      // add probe

Small nit: unnecessary comment.

@@ +288,5 @@
>        DebuggerView.Sources.selectedValue = aSource.url;
>        DebuggerController.Breakpoints.updateEditorBreakpoints();
>  
> +      // add probe
> +      if(histogram){

Small style nit: please add spaces after "if" before the opening paren, and after the closing paren, before the opening curly.

Comment 15

[Hubert B Manilla]

yeah, i'm taking out the try catch before we land.
thanks for the feedback.

Comment 16

[Hubert B Manilla]

Attachment: Added telemetry probes for how long it takes to display a selected source text to the user

Changes based on feedback

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 806973 [details] [diff] [review]
Added telemetry probes for how long it takes to display a selected source text to the user

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

Doesn't apply on the latest fx-team for me, needs to be rebased.

I will give review+ once you upload the updated patch and I can give the patch a spin for myself :)

::: browser/devtools/debugger/debugger-view.js
@@ +248,5 @@
>     * @param object aSource
>     *        The source object coming from the active thread.
>     */
>    set editorSource(aSource) {
> +    let histogram, startTime, transportType;

Small style nit: don't pre-declare variables, just do it as you use them since you aren't assigning to them in a different block from the block they are declared in. Also, you are re-declaring transportType below anyways.

@@ +286,5 @@
>        // Synchronize any other components with the currently displayed source.
>        DebuggerView.Sources.selectedValue = aSource.url;
>        DebuggerController.Breakpoints.updateEditorBreakpoints();
>  
> +      if (histogram) {

Since we are always creating the histogram, we don't need this if check.

Comment 18

[Hubert B Manilla]

Attachment: Added telemetry probes for how long it takes to display a selected source text to the user

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 807487 [details] [diff] [review]
Added telemetry probes for how long it takes to display a selected source text to the user

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

Looks like a bad rebase, will look at it again when you figure all that stuff out. If you need help determining which bits of code came from where and what should be kept or not, feel free to ping me on IRC or send an email or whatever.

::: browser/devtools/debugger/debugger-view.js
@@ +308,5 @@
>          return;
>        }
> +      this.editor.setText(aText);
> +      this.editor.resetUndo();
> +      this.setEditorMode(aSource.url, aSource.contentType, aText);

What is this new code? Why is this here?

@@ +314,5 @@
>        this._setEditorText(aText);
>        this._setEditorMode(aSource.url, aSource.contentType, aText);
> +      // Update the editor's current caret and debug locations given by the
> +      // currently active frame in the stack, if there's one available.
> +      this.updateEditor();

Ditto.

@@ +327,5 @@
> +
> +      histogram.add(+new Date() - startTime);
> +
> +      // Notify that we've shown a source file.
> +      window.dispatchEvent(document, "Debugger:SourceShown", aSource);

I think you have a bad rebase, I believe this was replaced with the window.emit above.

Comment 20

[Hubert B Manilla]

Thanks nick, 
i figured something was wrong with the rebase, but was not too sure.
Am sorting it out.
Rookie issues.

Comment 21

[Hubert B Manilla]

Attachment: Added telemetry probes to measure how long it takes to display a selected source text to the user

Fixed the issues.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 808145 [details] [diff] [review]
Added telemetry probes to measure how long it takes to display a selected source text to the user

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

LGTM. Made a small edit to DebuggerClient#localTransport to match DebuggerClient.requester, because it was report remote transports on a local one, but this was a tiny edit.

https://hg.mozilla.org/integration/fx-team/rev/6325295b03c9

Thanks for the patch :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6325295b03c9

Comment 24

[Victor Porof [:vporof][:vp]]

Comment on attachment 808145 [details] [diff] [review]
Added telemetry probes to measure how long it takes to display a selected source text to the user

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

::: browser/devtools/debugger/debugger-view.js
@@ +36,5 @@
>    showOverviewRuler: true
>  };
>  
> +//For telemetry
> +Cu.import("resource://gre/modules/Services.jsm")

You don't need this here, it's imported in debugger-controller.js, which shares the same scope.

@@ +287,5 @@
>        return this._editorSource.promise;
>      }
> +    let transportType = DebuggerController.client.localTransport
> +      ? "_LOCAL"
> +      : "_REMOTE";

Could use gClient for nicer formatting.

@@ +291,5 @@
> +      : "_REMOTE";
> +    //Telemetry probe
> +    let histogramId = "DEVTOOLS_DEBUGGER_DISPLAY_SOURCE" + transportType + "_MS";
> +    let histogram = Services.telemetry.getHistogramById(histogramId);
> +    let startTime = +new Date();

Date.now() is more appropriate.

@@ +313,5 @@
>        DebuggerView.Sources.selectedValue = aSource.url;
>        DebuggerController.Breakpoints.updateEditorBreakpoints();
>  
> +      histogram.add(+new Date() - startTime);
> +

Ditto.

Comment 25

[Victor Porof [:vporof][:vp]]

Attachment: Few small followups

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 809908 [details] [diff] [review]
Few small followups

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

Thanks, Victor :)

Comment 27

[Victor Porof [:vporof][:vp]]

Try was green: https://tbpl.mozilla.org/?tree=Try&rev=88b14648c12c

Comment 28

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/1fd8a49e4c2a

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1fd8a49e4c2a