710562 - Telemetry shouldn't have an ifdef MOZ_THUNDERBIRD in it

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect)

Description

[Mark Banner (:standard8)]

The general rule is that we don't have app-specific ifdefs in core. This tends to break things like other applications and building xulrunner style if we ever get that far. We've been working towards removing them for a while now, but we're not quite there yet.

The MOZ_THUNDERBIRD ifdef in TelementryHistograms.h shouldn't have been added:

http://hg.mozilla.org/mozilla-central/annotate/221eccfa6a3f/toolkit/components/telemetry/TelemetryHistograms.h#l259

Can we make those definitions global to core, or make it so that non-FF apps can hook in their own definitions in some manner?

Comment 1

[(dormant account)]

Can you #include a thunderdbird-specific header? we could do something like #include "telemetry_local.h" which would be provided from app-specific include search path.

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to Mark Banner (:standard8) from comment #0)
> Can we make those definitions global to core, ...?

That seems a possible workaround, though far from ideal :-|


(In reply to Taras Glek (:taras) from comment #1)
> Can you #include a thunderdbird-specific header?

Iiuc, "xulrunner style" would need a runtime solution (like preferences/properties/...) :-/

Ftr, Firefox has the same issue (if it cares about xulrunner):

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistograms.h
{
256 /**
257  * Thunderbird-specific telemetry.
258  */
259 #ifdef MOZ_THUNDERBIRD
260 HISTOGRAM(THUNDERBIRD_GLODA_SIZE_MB, 1, 1000, 40, LINEAR, "Gloda: size of global-messages-db.sqlite (MB)")
261 HISTOGRAM(THUNDERBIRD_CONVERSATIONS_TIME_TO_2ND_GLODA_QUERY_MS, 1, 10000, 30, EXPONENTIAL, "Conversations: time between the moment we click and the second gloda query returns (ms)")
262 HISTOGRAM(THUNDERBIRD_INDEXING_RATE_MSG_PER_S, 1, 100, 20, LINEAR, "Gloda: indexing rate (message/s)")
263 #endif
264 
265 /**
266  * Firefox-specific telemetry.
267  */
268 #ifdef MOZ_PHOENIX
269 HISTOGRAM(FX_CONTEXT_SEARCH_AND_TAB_SELECT, 0, 1, 2, BOOLEAN, "Firefox: Background tab was selected within 5 seconds of searching from the context menu")
270 #endif
}

Comment 3

[(dormant account)]

If you want a runtime solution, we'll have to wait until we do addon telemetry

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Disable 2 application-specific '#ifdef' in Toolkit, as a workaround ftb

(In reply to Mark Banner (:standard8) from comment #0)
> Can we make those definitions global to core

Taras:
(Untested yet) Would something like this be acceptable ftb?

Mark:
*'THUNDERBIRD_CONVERSATIONS_TIME_TO_2ND_GLODA_QUERY_MS' has been unused since it landed in bug 681873. In case you would want to remove it...
*Letting TB telemetry code succeed won't "harm" SM and/or TB, right?


(In reply to Taras Glek (:taras) from comment #3)
> If you want a runtime solution, we'll have to wait until we do addon
> telemetry

Is there a bug filed?

Comment 5

[Jonathan Protzenko [:protz]]

Thunderbird Conversations is used, as implied by the name, by Thunderbird Conversations.

https://addons.mozilla.org/en-US/thunderbird/addon/54035/

Serge, can you add a comment so that it isn't removed by mistake?

Comment 6

[Jonathan Protzenko [:protz]]

Sorry I meant THUNDERBIRD_CONVERSATIONS_TIME_TO_2ND_GLODA_QUERY_MS is used by Thunderbird Conversations.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Disable 2 application-specific '#ifdef' in Toolkit, as a workaround ftb [Checked in: Comments 10 and 18]

Av1, with comment 5 suggestion(s).

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 586103 [details] [diff] [review]
(Av1a) Disable 2 application-specific '#ifdef' in Toolkit, as a workaround ftb
[Checked in: Comments 10 and 18]

Letting TB telemetry code succeed in SM won't "harm" SM and/or TB, right?

Comment 9

[Jonathan Protzenko [:protz]]

No, because we can filter on the application in the telemetry dashboard, so we'll still be filtering on Thunderbird, don't wory :)

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 586103 [details] [diff] [review]
(Av1a) Disable 2 application-specific '#ifdef' in Toolkit, as a workaround ftb
[Checked in: Comments 10 and 18]

https://hg.mozilla.org/mozilla-central/rev/75ead35a1230


Ftr,
https://tbpl.mozilla.org/?tree=Try&rev=47e7f5360153
succeeded


(In reply to Jonathan Protzenko [:protz] from comment #9)
> No, because we can filter on the application in the telemetry dashboard

Thanks for confirmation.

***

[Approval Request Comment]
Regression caused by (bug #): bug 681873
User impact if declined: (iiuc) in SeaMonkey, (perma-orange xpcshell and) 1 console warning (maybe repeated) and no Gloda (nor Conversations) telemetry.
Testing completed (on m-c, etc.): yes, this comment.
Risk to taking this patch (and alternatives if risky): No risk, just a very small Toolkit code bloat.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to Taras Glek (:taras) from comment #3)
> If you want a runtime solution, we'll have to wait until we do addon
> telemetry

Bug 715927.

Comment 12

[Alex Keybl [:akeybl]]

(In reply to Serge Gautherie (:sgautherie) from comment #10)
> Risk to taking this patch (and alternatives if risky): No risk, just a very
> small Toolkit code bloat.

Since this code is new to Firefox at runtime, I'd like to let it bake on m-c for a couple of days before uplifting. I'll hold this in the queue.

Comment 13

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 710494 comment 4.
(FF and TB are still fine, SM is fixed.)

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 586103 [details] [diff] [review]
(Av1a) Disable 2 application-specific '#ifdef' in Toolkit, as a workaround ftb
[Checked in: Comments 10 and 18]

[Triage Comment]
Approving for Aurora/Beta since this is a low-risk Telemetry change and has been tested on m-c.

Comment 15

[Serge Gautherie (:sgautherie)]

Mark, why did you remove my checkin-needed?

Comment 16

[Mark Banner (:standard8)]

(In reply to Serge Gautherie (:sgautherie) from comment #15)
> Mark, why did you remove my checkin-needed?

Sorry, I misread the whiteboard.

Comment 17

[Serge Gautherie (:sgautherie)]

mozilla-aurora automatically updated with its mozilla11->mozilla12 switch.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 586103 [details] [diff] [review]
(Av1a) Disable 2 application-specific '#ifdef' in Toolkit, as a workaround ftb
[Checked in: Comments 10 and 18]

http://hg.mozilla.org/releases/mozilla-beta/rev/5ad575739e86
(adapted; FX_KEYWORD_URL_USERSET is also present on Aurora)

Comment 19

[Serge Gautherie (:sgautherie)]

firefox11: verified, per bug 710494 comment 5.