Closed Bug 907755 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: fitzgen, Assigned: b4bomsy)

Details

(Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js])

Attachments

(2 files, 3 obsolete files)

Should start timing when the user selects an unfetched source and shouldn't report the probe until right after the source text is displayed.
This would be a good bug for someone who has already completed one or two other debugger bugs.
Priority: -- → P3
Whiteboard: [mentor=nfitzgerald@mozilla.com][lang=js]
Hi nick,
Would like to work on this.
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.
Assignee: nobody → b4bomsy
Status: NEW → ASSIGNED
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.
ok..thanks. on it
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?
(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}.
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.
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.
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.
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
it worked, it needed a full build. thanks.
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
Attachment #804932 - Flags: feedback?(nfitzgerald)
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.
Attachment #804932 - Flags: feedback?(nfitzgerald) → feedback+
yeah, i'm taking out the try catch before we land.
thanks for the feedback.
Changes based on feedback
Attachment #804932 - Attachment is obsolete: true
Attachment #806973 - Flags: review?(nfitzgerald)
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.
Attachment #806973 - Flags: review?(nfitzgerald) → feedback+
Attachment #806973 - Attachment is obsolete: true
Attachment #807487 - Flags: feedback?(nfitzgerald)
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.
Attachment #807487 - Flags: feedback?(nfitzgerald)
Thanks nick, 
i figured something was wrong with the rebase, but was not too sure.
Am sorting it out.
Rookie issues.
Fixed the issues.
Attachment #807487 - Attachment is obsolete: true
Attachment #808145 - Flags: review?(nfitzgerald)
Attachment #808145 - Flags: feedback?(nfitzgerald)
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 :)
Attachment #808145 - Flags: review?(nfitzgerald)
Attachment #808145 - Flags: review+
Attachment #808145 - Flags: feedback?(nfitzgerald)
Whiteboard: [mentor=nfitzgerald@mozilla.com][lang=js] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6325295b03c9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Target Milestone: --- → Firefox 27
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.
Attachment #809908 - Flags: review?(nfitzgerald)
Comment on attachment 809908 [details] [diff] [review]
Few small followups

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

Thanks, Victor :)
Attachment #809908 - Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/integration/fx-team/rev/1fd8a49e4c2a
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1fd8a49e4c2a
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.