Metadata ping to AMO for a system add-on seems un-necessary

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: andym, Assigned: mossop)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
Created attachment 8859642 [details]
Screenshot-2017-4-19 Addons Prod Datadog.png

When a system add-on is deployed, it does an install in Firefox. That install then triggers a ping to AMO to get any metadata. So when the add-on in bug 1356676 was deployed to Firefox, there was a corresponding increase in traffic to the endpoint:

https://services.addons.mozilla.org/%LOCALE%/firefox/api/%API_VERSION%/search/guid:%IDS%?src=firefox&appOS=%OS%&appVersion=%VERSION%

for example:

.../es/firefox/api/1.5/search/guid%3Adisable-cert-transparency%40mozilla.org?src=firefox&appOS=WINNT&appVersion=52.0.2

And I'll attach a graph.

That metadata ping could (theoretically) be used to load compatibility data from AMO, but this seems redundant in general because the main tool for deploying and maintaining system add-ons is balrog. Having two systems balrog and AMO both trying to do that seems like a recipe for failure.

So I suggest that if its a system add-on, let's not ping AMO for that data.
(Assignee)

Updated

7 months ago
Assignee: nobody → dtownsend
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8859719 [details]
Bug 1357804: Stop making metadata requests to AMO for system add-ons.

https://reviewboard.mozilla.org/r/131722/#review134556

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:142
(Diff revision 1)
>  
> -      // The add-ons manager may not know about this ID yet if it is a pending
> +    // The add-ons manager may not know about this ID yet if it is a pending
> -      // install. In that case we'll just cache it regardless
> +    // install. In that case we'll just cache it regardless
> -      if (!addon || types.includes(addon.type))
> +
> +    // Don't cache add-ons of the wrong types
> +    if (addon && !types.includes(addon.type))

Nit: please use braces. I'd like to enable the "no-lonely-if" eslint rule soon.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:146
(Diff revision 1)
> +    // Don't cache add-ons of the wrong types
> +    if (addon && !types.includes(addon.type))
> +      continue;
> +
> +    // Don't cache system add-ons
> +    if (addon && addon.isSystem)

Here too.
Attachment #8859719 - Flags: review?(rhelmer) → review+
Comment hidden (mozreview-request)

Comment 4

7 months ago
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c474d4b27bfd
Stop making metadata requests to AMO for system add-ons. r=rhelmer

Comment 5

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c474d4b27bfd
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.