Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

update to new Google search codes (fixes issue #885) #916

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

@cvan cvan self-assigned this Jan 3, 2019
@cvan cvan added this to the v1.2 milestone Jan 3, 2019
Copy link

@mkaply mkaply left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like we got these right the first time. keyword should be client. And the ie and oe can be removed.

@mkaply
Copy link

mkaply commented Jan 3, 2019

Can you show me where this code is used? Does Reality report anything search related to telemetry?

@mkaply
Copy link

mkaply commented Jan 7, 2019

We need to get this fixed ASAP if we are getting more distribution.

@avrignaud
Copy link

avrignaud commented Jan 7, 2019 via email

@cvan
Copy link
Contributor Author

cvan commented Jan 8, 2019

Can you show me where this code is used? Does Reality report anything search related to telemetry?

Yes, our Telemetry pings suggest that we are using a variety of search engines. (MoCo-Private Telemetry Query Dashboard: https://sql.telemetry.mozilla.org/queries/58566#151919)

We need to get this fixed ASAP if we are getting more distribution.

@mkaply: per your review comment #916 (review), I made those adjustments to this PR. does it look good now?

P.S. I'm not familiar with our search arrangement, but I'll help make sure we ship out a new version ASAP once this is merged. also, I'm not sure of the context, but @larsbergstrom can probably coordinate the logistics to make sure the malformed search queries can still been accounted for.

@cvan cvan modified the milestones: v1.2, v1.1.2 Jan 8, 2019
@larsbergstrom
Copy link

@MortimerGoro Can you please take a look at this code? It looks good to me.

@cvan @mkaply Unfortunately, we don't have any insights beyond our telemetry dashboard that tracks the queries performed to be able to see on the "backend" whether our search queries are working correctly or not, since it's aggregated with all Fx products sharing those codes.

@mkaply
Copy link

mkaply commented Jan 8, 2019

@cvan @mkaply Unfortunately, we don't have any insights beyond our telemetry dashboard that tracks the queries performed to be able to see on the "backend" whether our search queries are working correctly or not, since it's aggregated with all Fx products sharing those codes.

That's actually what we need. Can we get access to your search telemetry data so we can integrate into the new dashboards once these codes are live?

@MortimerGoro
Copy link
Contributor

@larsbergstrom LGTM

@larsbergstrom
Copy link

That's actually what we need. Can we get access to your search telemetry data so we can integrate into the new dashboards once these codes are live?

If you check out the link @cvan provided to the internal query, in theory our data will just show up in any dashboard you write against Firefox search telemetry, as we are using the same reporting, but with app_name in ('FirefoxReality_wavevr', 'FirefoxReality_oculusvr', 'FirefoxReality_googlevr').

Feel free to hit us up on Slack if you have more detailed questions on that bit!

@cvan cvan merged commit d7b89d7 into master Jan 8, 2019
@cvan cvan deleted the new-google-search-code branch January 8, 2019 20:00
@cvan
Copy link
Contributor Author

cvan commented Jan 9, 2019

@MozillaReality/fxr-eng @larsbergstrom @philip-lamb: now that this PR is merged to master, shall we cut a v1.1.2 release for this?

@larsbergstrom
Copy link

@cvan Yes, we'll probably want both this fix and the one for bookmarks, which was blocking an update to Firefox Reality in the HTC store (they caught it during review!).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants