Last Comment Bug 905759 - move telemetry sending from java into native code
: move telemetry sending from java into native code
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 26
Assigned To: Nobody; OK to take it and work on it
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-15 12:33 PDT by Nathan Froyd [:froydnj]
Modified: 2013-08-23 15:00 PDT (History)
5 users (show)
nfroyd: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - add Telemetry infrastructure for telemetry gecko events (2.25 KB, patch)
2013-08-15 12:34 PDT, Nathan Froyd [:froydnj]
vladan.bugzilla: review+
Details | Diff | Splinter Review
part 2 - add Telemetry-specific GeckoEvent and related machinery (5.61 KB, patch)
2013-08-15 12:35 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
Details | Diff | Splinter Review
part 3 - delete browser.js code for receiving telemetry events from Java (2.75 KB, patch)
2013-08-15 12:36 PDT, Nathan Froyd [:froydnj]
mark.finkle: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2013-08-15 12:33:56 PDT
We shouldn't need to deal with JSON for this; even though it's a small amount, even
this little bit of JSON shows up at 1% or so on traces for startup times.  Also reduces
the amount of work we have to do in Javascript, which is a win.
Comment 1 Nathan Froyd [:froydnj] 2013-08-15 12:34:51 PDT
Created attachment 790907 [details] [diff] [review]
part 1 - add Telemetry infrastructure for telemetry gecko events

Java doesn't have access to histogram IDs, so we need some way of adding samples to
histograms by string ID.  I hope the comment is sufficient to deter people from using
the string Accumulate...
Comment 2 Nathan Froyd [:froydnj] 2013-08-15 12:35:30 PDT
Created attachment 790909 [details] [diff] [review]
part 2 - add Telemetry-specific GeckoEvent and related machinery

This patch encompasses all the Java-side and widget-side machinery for telemetry
events.  Pretty straightforward.
Comment 3 Nathan Froyd [:froydnj] 2013-08-15 12:36:32 PDT
Created attachment 790910 [details] [diff] [review]
part 3 - delete browser.js code for receiving telemetry events from Java

We don't need most of this code anymore, so let's delete it.  I left the Telemetry
object for the sake of not touching lots and lots of the file and that it's a little
clearer, IMHO.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2013-08-15 12:41:30 PDT
Comment on attachment 790909 [details] [diff] [review]
part 2 - add Telemetry-specific GeckoEvent and related machinery

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

yes please, more of this
Comment 5 Vladan Djeric (:vladan) 2013-08-18 20:26:20 PDT
Comment on attachment 790907 [details] [diff] [review]
part 1 - add Telemetry infrastructure for telemetry gecko events

>+  Histogram *h;
>+  rv = GetHistogramByEnumId(id, &h);
>+  if (NS_SUCCEEDED(rv))
>+    h->Add(sample);

Nit: add curlies around the if

> /**
>+ * Adds a sample to a histogram defined in TelemetryHistograms.h.
>+ * You should use the by-enum-id version defined above unless you have
>+ * a really good reason to use this one.
>+ *
>+ * @param name - histogram name
>+ * @param sample - value to record
>+ */
>+void Accumulate(const char* name, uint32_t sample);

Explain why this function even exists, so readers have a better idea of what a really good reason might be
Comment 6 Nathan Froyd [:froydnj] 2013-08-22 08:23:51 PDT
Comment on attachment 790910 [details] [diff] [review]
part 3 - delete browser.js code for receiving telemetry events from Java

Bouncing this to Brian.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2013-08-22 08:31:00 PDT
Comment on attachment 790910 [details] [diff] [review]
part 3 - delete browser.js code for receiving telemetry events from Java

Sorry I missed this review. Looks good to me.

Note You need to log in before you can comment on or make changes to this bug.