Make min/max/bucket_count optional for nsITelemetry newHistogram()/registerAddonHistogram()

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

({addon-compat})

unspecified
mozilla36
Points:
3
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed, firefox36 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Per IRC registerAddonHistogram() is not used by anyone, so we can actually move the min/max/bucket_count arguments to be last and make them optional.
Points: --- → 3
Are you sure? I'm about 99% sure that bwinton uses it for whimsy.
Flags: needinfo?(bwinton)
Jorge, can you check on AMO whether nsITelemetry.registerAddonHistogram() has any relevant users?
Flags: needinfo?(jorge)
Depends on: 1069990
I think we should be good for a decision with the data from bug 1069990 here.
Flags: needinfo?(jorge)
I don't use it in whimsy.  Like any other bad developer, I just stuff all my data into the UITelemetry JSON blob.  ;)
Flags: needinfo?(bwinton)
I don't see any consumers in the add-ons MXR. Regardless, I'll mark this bug with the addon-compat keyword so that we at least mention it in the compatibility blog post.
Keywords: addon-compat
Thanks.
Per bug 1069990 only pdf.js and shumway seem to use this, which are our projects - so this should be fine.
Depends on: 1076897
Depends on: 1076909
Nathan, how does this look to you?

This part is just changing the interface and its implementation, the callers will come in a different patch.
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Attachment #8499008 - Flags: review?(nfroyd)
Comment on attachment 8499008 [details] [diff] [review]
Part 1: Make min/max/bucket_count optional

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

Makes sense to me, with the change below.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1520,3 @@
>                                        uint32_t min, uint32_t max,
>                                        uint32_t bucketCount,
> +                                      uint8_t optArgCount)

I think you need checks in this function (or in CreateHistogramForAddon) verifying that optArgCount is appropriate for whatever histogramType requires.  Otherwise, you could call:

  .registerAddonHistogram(..., "bogus", HISTOGRAM_EXPONENTIAL);

and wind up with garbage.
Attachment #8499008 - Flags: review?(nfroyd) → review+
Fixed the above and also added sanity checks on the parameters while we are there.
Attachment #8499008 - Attachment is obsolete: true
Attachment #8503206 - Flags: review+
Posted patch Part 2: Fixup tests. (obsolete) — Splinter Review
Attachment #8503208 - Flags: review?(nfroyd)
Attachment #8503208 - Flags: review?(nfroyd) → review+
I split up the patches differently, with the newHistogram() changes in part 1 and the registerAddonHistogram() changes in part 2.
This is so i can base bug 1069874 on part 1 without blocking on the addon part.
Attachment #8503206 - Attachment is obsolete: true
Attachment #8503208 - Attachment is obsolete: true
Attachment #8507003 - Flags: review+
Blocks: 1076897
No longer depends on: 1076897
Comment on attachment 8507003 [details] [diff] [review]
Part 1: Make min/max/bucket_count optional on newHistogram()

Approval Request Comment
[Feature/regressing bug #]: Search telemetry.
[User impact if declined]:
[Describe test coverage new/current, TBPL]: Good automated test-coverage.
[Risks and why]: Low risk - good test-coverage.
[String/UUID change made/needed]: None.
Attachment #8507003 - Flags: approval-mozilla-beta?
Attachment #8507003 - Flags: approval-mozilla-aurora?
Comment on attachment 8507003 [details] [diff] [review]
Part 1: Make min/max/bucket_count optional on newHistogram()

Aurora+

Moving Beta approval request to Beta rebase patch.
Attachment #8507003 - Flags: approval-mozilla-beta?
Attachment #8507003 - Flags: approval-mozilla-aurora?
Attachment #8507003 - Flags: approval-mozilla-aurora+
Comment on attachment 8515324 [details] [diff] [review]
Beta rebase - Part 1: Make min/max/bucket_count optional on newHistogram()

This patch includes an UUID change. Is there a way to rewrite this patch that doesn't require an UUID change? If not, how do you expect this to impact add-ons?
Flags: needinfo?(georg.fritzsche)
Attachment #8515324 - Flags: approval-mozilla-beta?
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #19)
> Comment on attachment 8515324 [details] [diff] [review]
> Beta rebase - Part 1: Make min/max/bucket_count optional on newHistogram()
> 
> This patch includes an UUID change. Is there a way to rewrite this patch
> that doesn't require an UUID change? If not, how do you expect this to
> impact add-ons?

This (and the dependent patches in the queue) are not trivial to rewrite without changing the interface.
The affected methods in "part 1" are not for use by addons (i kept those changes to "part 2").

Per bug 1069990 we only have Shumway & Pdf.js submitting Telemetry, both of which are not using binary components.
From my understanding that means that we won't impact them - but i'm not entirely sure about this.
Flags: needinfo?(georg.fritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> Per bug 1069990 we only have Shumway & Pdf.js submitting Telemetry, both of
> which are not using binary components.
> From my understanding that means that we won't impact them - but i'm not
> entirely sure about this.

This was confirmed on IRC by smaug and i cross-checked the sources for both addons to confirm that they are not affected.
Comment on attachment 8515324 [details] [diff] [review]
Beta rebase - Part 1: Make min/max/bucket_count optional on newHistogram()

Approving for Beta as per comment 20 and comment 21 this UUID bump is not expected to have any impact on add-ons.

Jorge - Adding ni on you for awareness in case you want to communicate the Telemetry related changes to add-on authors.
Flags: needinfo?(jorge)
Attachment #8515324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1076909
No longer depends on: 1076909
Talking to Yury on IRC the patches for Shumway & PDF.js are ready, so he is ok with landing this now.

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfbdd6d0a996
(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> Talking to Yury on IRC the patches for Shumway & PDF.js are ready, so he is
> ok with landing this now.

... "this" being Part 2 with the registerAddonHistogram() changes.
Keywords: leave-open
We haven't done Firefox 34 outreach yet, and I don't expect those components to be accessed from binary add-ons, so I think we're good.
Flags: needinfo?(jorge)
https://hg.mozilla.org/mozilla-central/rev/dfbdd6d0a996
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Is there more being uplifted to Aurora/Beta here or are we done?
Flags: needinfo?(georg.fritzsche)
This bug is done, we only needed to uplift part 1 here.
Flags: needinfo?(georg.fritzsche)
Calling 34 and 35 fixed for tracking purposes then.
You need to log in before you can comment on or make changes to this bug.