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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Benjamin Smedberg]

Are you sure? I'm about 99% sure that bwinton uses it for whimsy.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Jorge, can you check on AMO whether nsITelemetry.registerAddonHistogram() has any relevant users?

Comment 3

[Georg Fritzsche [:gfritzsche]]

I think we should be good for a decision with the data from bug 1069990 here.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

I don't use it in whimsy.  Like any other bad developer, I just stuff all my data into the UITelemetry JSON blob.  ;)

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Thanks.
Per bug 1069990 only pdf.js and shumway seem to use this, which are our projects - so this should be fine.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Make min/max/bucket_count optional

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.

Comment 8

[Nathan Froyd [:froydnj]]

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.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Make min/max/bucket_count optional

Fixed the above and also added sanity checks on the parameters while we are there.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2: Fixup tests.

Comment 11

[Georg Fritzsche [:gfritzsche]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8a4841dfffa0
https://tbpl.mozilla.org/?tree=Try&rev=8a4841dfffa0

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Make min/max/bucket_count optional on newHistogram()

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.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2: Make min/max/bucket_count optional on registerAddonHistogram()

Comment 14

[Georg Fritzsche [:gfritzsche]]

Part 1 landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba9f0fcad00

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ba9f0fcad00

Comment 16

[Georg Fritzsche [:gfritzsche]]

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.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Attachment: Beta rebase - Part 1: Make min/max/bucket_count optional on newHistogram()

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 20

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 21

[Georg Fritzsche [:gfritzsche]]

(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 22

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c899386b250c

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 24

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56b3e37832b9

Comment 25

[Georg Fritzsche [:gfritzsche]]

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

Comment 26

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 27

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfbdd6d0a996

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Is there more being uplifted to Aurora/Beta here or are we done?

Comment 30

[Georg Fritzsche [:gfritzsche]]

This bug is done, we only needed to uplift part 1 here.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Calling 34 and 35 fixed for tracking purposes then.