Closed Bug 850738 Opened 11 years ago Closed 11 years ago

Add a crap load of telemetry tests

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: fitzgen, Assigned: fitzgen)

Details

Attachments

(1 file, 5 obsolete files)

We can easily add a crap load of telemetry tests by making a higher order function for clients to use which also takes a telemetry id to automatically time stuff.
Attached patch v2 (obsolete) — Splinter Review
Using performance.now() instead of +new Date
Attachment #724721 - Attachment is obsolete: true
Attachment #724721 - Flags: review?(past)
Attachment #726856 - Flags: review?(past)
Comment on attachment 726856 [details] [diff] [review]
v2

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

While testing the previous version of this patch I couldn't see any histograms appearing in about:telemetry. Why is that?

When applying this on top of bug 850233 I get:
ReferenceError: performance is not defined: DC_request@resource://gre/modules/devtools/dbg-client.jsm:521

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +27,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "WebConsoleClient",
>                                    "resource://gre/modules/devtools/WebConsoleClient.jsm");
>  
> +let Telemetry = Services.telemetry;

Since this is only used once, let's inline it.

@@ +226,5 @@
>  
> +/**
> + * A declarative method-creater for sending requests to the server.
> + *
> + * @param aPacket

The argument name is declared as aPacketSkeleton below.

@@ +230,5 @@
> + * @param aPacket
> + *        The form of the packet to send. Can specify fields to be filled from
> + *        the parameters by using the |params| function.
> + * @param to
> + *        The actor to send the request to.

|to| is a property of aPacketSkeleton.
Attachment #726856 - Flags: review?(past)
Attached patch v3 (obsolete) — Splinter Review
Woops, fixed a bad conditional that caused the telemetry histogram to never be grabbed.

Back to using +new Date. Sorry about all the fuss.

Confirmed that the data was being aggregated in about:telemetry.
Attachment #726856 - Attachment is obsolete: true
Attachment #726907 - Flags: review?(past)
Comment on attachment 726907 [details] [diff] [review]
v3

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

I expect that people who do both local and remote debugging will get twin peaks in their histograms, and less accuracy as a result. What do you think about using different histograms for each case depending on the type of DebuggerClient.prototype._transport?

IIRC you wanted to convert all the public API calls to use telemetry, which is why I'm wondering how come you didn't include the |attach| requests to the tab and thread actors, or the detach from the thread actor request. The latter has been made significantly more efficient in a previous patch, and I wish I had telemetry back then to measure how much. On a similar note you included the |parameterNames|, |property| and |prototype| requests, which are not used by the current debugger frontend.

Another one that I think should be included in telemetry data is |source|, since we'd like to have an estimate of how slow it is in real world usage, in order to prioritize the work to read the source in chunks. The same holds for |setBreakpoint|, it is code that we have been constantly refactoring in the past.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +226,5 @@
> + * A declarative method-creater for sending requests to the server.
> + *
> + * @param aPacketSkeleton
> + *        The form of the packet to send. Can specify fields to be filled from
> + *        the parameters by using the |params| function.

What do you think about using args instead of params to better convey its array-like nature?

@@ +239,5 @@
> + *        will be passed to the callback.
> + * @param callback
> + *        By default the callback is considered to be the last parameter. If you
> + *        want the callback to be any other parameter, you can specify which one
> + *        here with the |params| function.

The only case where the callback isn't the last parameter is resume(), right? Instead of complicating this function further, why not fix the few stepping call sites of resume() to pass the callback after the limit? The DebuggerClient.Param handling is confusing enough as it is.

@@ +278,5 @@
> +          aResponse.from = from;
> +        }
> +      }
> +
> +      // Unless the callback was specified to be in at certain parameter

Typo.
Attachment #726907 - Flags: review?(past)
(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 726907 [details] [diff] [review]
> v3
> 
> Review of attachment 726907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I expect that people who do both local and remote debugging will get twin
> peaks in their histograms, and less accuracy as a result. What do you think
> about using different histograms for each case depending on the type of
> DebuggerClient.prototype._transport?

Yeah we could do this. Maybe rather than passing the whole telemetry probe name, we can just do the unique suffix, and then automatically fetch prepend that with LOCAL or REMOTE based on the transport.

> IIRC you wanted to convert all the public API calls to use telemetry, which
> is why I'm wondering how come you didn't include the |attach| requests to
> the tab and thread actors, or the detach from the thread actor request. The
> latter has been made significantly more efficient in a previous patch, and I
> wish I had telemetry back then to measure how much. On a similar note you
> included the |parameterNames|, |property| and |prototype| requests, which
> are not used by the current debugger frontend.

Basically I just did all the low hanging fruit. IIRC, it was harder to fit the attach and detach functions into the |requester| paradigm. I will revisit this.

> 
> Another one that I think should be included in telemetry data is |source|,
> since we'd like to have an estimate of how slow it is in real world usage,
> in order to prioritize the work to read the source in chunks. The same holds
> for |setBreakpoint|, it is code that we have been constantly refactoring in
> the past.
> 
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +226,5 @@
> > + * A declarative method-creater for sending requests to the server.
> > + *
> > + * @param aPacketSkeleton
> > + *        The form of the packet to send. Can specify fields to be filled from
> > + *        the parameters by using the |params| function.
> 
> What do you think about using args instead of params to better convey its
> array-like nature?

Sure.

> @@ +239,5 @@
> > + *        will be passed to the callback.
> > + * @param callback
> > + *        By default the callback is considered to be the last parameter. If you
> > + *        want the callback to be any other parameter, you can specify which one
> > + *        here with the |params| function.
> 
> The only case where the callback isn't the last parameter is resume(),
> right? Instead of complicating this function further, why not fix the few
> stepping call sites of resume() to pass the callback after the limit? The
> DebuggerClient.Param handling is confusing enough as it is.

I was under the impression that we wanted to avoid changing the debugger client api as much as possible. I can do that though and it would make |requester| a bit simpler.

What do you think?
So I remember why I didn't do attach, now: because it seemed very hard to make |requester| work with the way that attach sends extra arguments to the callback (the thread client). I could do something convoluted, but it seems gross. Thoughts?
(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> (In reply to Panos Astithas [:past] from comment #5)
> > @@ +239,5 @@
> > > + *        will be passed to the callback.
> > > + * @param callback
> > > + *        By default the callback is considered to be the last parameter. If you
> > > + *        want the callback to be any other parameter, you can specify which one
> > > + *        here with the |params| function.
> > 
> > The only case where the callback isn't the last parameter is resume(),
> > right? Instead of complicating this function further, why not fix the few
> > stepping call sites of resume() to pass the callback after the limit? The
> > DebuggerClient.Param handling is confusing enough as it is.
> 
> I was under the impression that we wanted to avoid changing the debugger
> client api as much as possible. I can do that though and it would make
> |requester| a bit simpler.
> 
> What do you think?

I think debugger client users are few at the moment, so we have some leeway to make API-breaking changes.

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> So I remember why I didn't do attach, now: because it seemed very hard to
> make |requester| work with the way that attach sends extra arguments to the
> callback (the thread client). I could do something convoluted, but it seems
> gross. Thoughts?

You make a good point and I don't think telemetry for attach requests is all that useful right now. I'm fine with whatever you think is best.
Status: NEW → ASSIGNED
> (In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> > So I remember why I didn't do attach, now: because it seemed very hard to
> > make |requester| work with the way that attach sends extra arguments to the
> > callback (the thread client). I could do something convoluted, but it seems
> > gross. Thoughts?
> 
> You make a good point and I don't think telemetry for attach requests is all
> that useful right now. I'm fine with whatever you think is best.

Follow up? I kinda just want to land this already.
> Follow up? I kinda just want to land this already.

Sure.
Attached patch v4 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=96cc21d60efb

* s/params/args/

* Removed option of specifying callback position in |requester|

* Changed |resume| so that the callback is last, and the |resumeType| is first
Attachment #726907 - Attachment is obsolete: true
Attachment #732024 - Flags: review?(past)
* Also, made it so that there are seperate probes for local and remote transports
Comment on attachment 732024 [details] [diff] [review]
v4

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

Polluting the client.resume() API with the implementation requirement to have a null first argument is suboptimal. Why not just add a private _resume or _doResume method that is burdened with that requirement, which both resume() and the stepping functions can call?

The B2G test breakage must be fixed. It looks like the first few tests passed, and those tests don't use the DebuggerClient at all, so it's highly likely that the patch is causing the failures.

Maybe telemetry isn't properly packaged in those builds? It looks like it is in b2g/installer/package-manifest.in, but maybe I'm reading it wrong, or maybe the test harness runs things differently. You could do a try run with these 2 lines removed to confirm my suspicion.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +235,5 @@
> + *        and the return value is used as the new packet.
> + * @param after
> + *        The function to call after the response is received. It is passed the
> + *        response, and the return value is considered the new response that
> + *        will be passed to the callback.

It would be good to note that |before| and |after| are called with the debugger client instance as the receiver.

@@ +281,5 @@
> +      }
> +
> +      // Unless the callback was specified to be at a certain parameter
> +      // position, we assume it is the last one.
> +      let thisCallback = args[maxPosition + 1];

The comment is no longer accurate, the callback is now always the last one.
Attachment #732024 - Flags: review?(past)
(In reply to Panos Astithas [:past] from comment #13)
> The B2G test breakage must be fixed. It looks like the first few tests
> passed, and those tests don't use the DebuggerClient at all, so it's highly
> likely that the patch is causing the failures.
> 
> Maybe telemetry isn't properly packaged in those builds? It looks like it is
> in b2g/installer/package-manifest.in, but maybe I'm reading it wrong, or
> maybe the test harness runs things differently. You could do a try run with
> these 2 lines removed to confirm my suspicion.

I can actually reproduce the failures locally:

ReferenceError: LocalDebuggerTransport is not defined

You just need to assign LocalDebuggerTransport to this.LocalDebuggerTransport I believe.
(In reply to Panos Astithas [:past] from comment #13)
> Comment on attachment 732024 [details] [diff] [review]
> v4
> 
> Review of attachment 732024 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Polluting the client.resume() API with the implementation requirement to
> have a null first argument is suboptimal. Why not just add a private _resume
> or _doResume method that is burdened with that requirement, which both
> resume() and the stepping functions can call?

Sorry I misunderstood your last review comments and thought that you were asking me to do that.
Attached patch v5 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=bb0c16cc8bc9

* Added comments about |this| in |before| and |after|

* Saved |LocalDebuggerTransport| to |this| so b2g doesn't freak out

* Fixed comment about |callback| argument positioning

* Made |resume| have the same old api, and created |_doResume| which uses the |requester| helper.
Attachment #732024 - Attachment is obsolete: true
Attachment #733589 - Flags: review?(past)
Attachment #733589 - Flags: review?(past)
Attached patch v6Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=7339c947aebf

* Added telemetry probes to Histograms.json that were missing
Attachment #733589 - Attachment is obsolete: true
Attachment #735385 - Flags: review?(past)
Attachment #735385 - Flags: review?(past) → review+
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/3cd8277947a6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: