SEARCH_DEFAULT_ENGINE probe only set during idle-daily!

RESOLVED FIXED in Firefox 36

Status

()

Toolkit
Telemetry
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vladan, Assigned: gfritzsche)

Tracking

(Blocks: 1 bug)

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
It looks like SEARCH_DEFAULT_ENGINE is only filled in during the "gather-telemetry" event which I believe is only fired after "idle-daily". 

This means that only one session during a 24 hour period will report the SEARCH_DEFAULT_ENGINE value.
(Reporter)

Comment 1

4 years ago
I believe other Telemetry probes also incorrectly depend on "gather-telemetry".. I will file other bugs once this one is confirmed
(Reporter)

Updated

4 years ago
Blocks: 1105864
(Assignee)

Updated

4 years ago
Assignee: nobody → georg.fritzsche
(Assignee)

Comment 2

4 years ago
Hm, ok :-/
We could just see that we submit this on (or close to) startup. Sound reasonable?
(Assignee)

Updated

4 years ago
Blocks: 1089670
(Assignee)

Comment 3

4 years ago
Created attachment 8534994 [details] [diff] [review]
Submit default search engine on startup too

Blake, does this seems reasonable?
Do you happen to know if we could have perf issues with touching the search service too early?
Attachment #8534994 - Flags: review?(bwinton)
Comment on attachment 8534994 [details] [diff] [review]
Submit default search engine on startup too

It seems okay to me, the one thing I'm worried about is that the search service might not be initialized at this point.  Should we wait for browser-search-service's init-complete notification (sent from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2989 and http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#3013)?

(You could try pinging florian or mconley for the answer to that question…)
Attachment #8534994 - Flags: review?(bwinton) → review+
Attachment #8534994 - Flags: review?(mconley)
(Assignee)

Comment 5

4 years ago
Hm, or, if search service init is a concern...
Maybe it makes more sense to move over to nsBrowserGlue.js and just hang off "browser-search-engine-modified" and "browser-search-service" here:
http://hg.mozilla.org/mozilla-central/annotate/0cf461e62ce5/browser/components/nsBrowserGlue.js#l390
(Assignee)

Comment 6

4 years ago
Florian or MattN, can you comment on comment 4 and comment 5 so i can do something here this week?
Flags: needinfo?(florian)
Flags: needinfo?(MattN+bmo)
Comment on attachment 8534994 [details] [diff] [review]
Submit default search engine on startup too

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

I agree that it makes more sense to collect this information in nsBrowserGlue in those observer notification handlers.
Attachment #8534994 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Flags: needinfo?(florian)
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 8

4 years ago
Created attachment 8537247 [details] [diff] [review]
Submit default search engine on search service events

Fixed this to hang off the referenced search service events.
Attachment #8534994 - Attachment is obsolete: true
Attachment #8537247 - Flags: review?(mconley)
Comment on attachment 8537247 [details] [diff] [review]
Submit default search engine on search service events

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

r=me with the bracing changes reverted.

::: browser/components/nsBrowserGlue.js
@@ +419,5 @@
>          // initialized already when this notification fires.
>          let ss = Services.search;
>          if (ss.currentEngine.name == ss.defaultEngine.name)
>            return;
> +        if (data == "engine-current") {

I understand the impulse to brace these - my instinct says the same thing. Unfortunately, the rest of this file uses the "no braces for one-liner blocks" convention, and we should probably stick to that convention (or change the entire file).
Attachment #8537247 - Flags: review?(mconley) → review+
(Assignee)

Comment 10

4 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> I understand the impulse to brace these - my instinct says the same thing.
> Unfortunately, the rest of this file uses the "no braces for one-liner
> blocks" convention, and we should probably stick to that convention (or
> change the entire file).

A little sad, but fair point.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

4 years ago
Benjamin, do we still have a need to uplift this to at least aurora?
Flags: needinfo?(benjamin)

Comment 13

4 years ago
Yes please.
Flags: needinfo?(benjamin)
(Assignee)

Comment 14

4 years ago
Comment on attachment 8537247 [details] [diff] [review]
Submit default search engine on search service events

Approval Request Comment
[Feature/regressing bug #]: Recording current search engine measurement in Telemetry.
[User impact if declined]: No useful search engine telemetry.
[Describe test coverage new/current, TBPL]: Manually verified, automated tests still wait for having time for them.
[Risks and why]: Low-risk, small change that just moves search telemetry around to a properly working place.
[String/UUID change made/needed]: None.
Attachment #8537247 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ba715f80ac5f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8537247 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.