The default bug view has changed. See this FAQ.

move telemetry sending from java into native code

RESOLVED FIXED in Firefox 26

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
Firefox 26
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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...
Attachment #790907 - Flags: review?(vdjeric)
(Reporter)

Comment 2

4 years ago
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.
Attachment #790909 - Flags: review?(blassey.bugs)
(Reporter)

Comment 3

4 years ago
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.
Attachment #790910 - Flags: review?(mark.finkle)
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
Attachment #790909 - Flags: review?(blassey.bugs) → review+
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
Attachment #790907 - Flags: review?(vdjeric) → review+
(Reporter)

Comment 6

4 years ago
Comment on attachment 790910 [details] [diff] [review]
part 3 - delete browser.js code for receiving telemetry events from Java

Bouncing this to Brian.
Attachment #790910 - Flags: review?(mark.finkle) → review?(bnicholson)
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.
Attachment #790910 - Flags: review?(bnicholson) → review+
(Reporter)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaadde1a566d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a0ad0c7f9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/134285f3187f

Mochitest-only try run: https://tbpl.mozilla.org/?tree=Try&rev=7957914f3c46
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/eaadde1a566d
https://hg.mozilla.org/mozilla-central/rev/d7a0ad0c7f9d
https://hg.mozilla.org/mozilla-central/rev/134285f3187f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.