1134550 - Serve suggested tiles with v3 endpoint while keeping v1/v2 compatibility

Status: RESOLVED FIXED

(Content Services Graveyard :: Tiles: Content Front-End, defect)

Description

[Ed Lee :Mardak]

The current patch in bug 1126182 assumes related tiles comes from the server like any other tile except with type="related" instead of type="sponsored".

{ "en-US": [{"title": "tile", "type": "related"} }

Except we want to support related sponsored tiles as well as related organic tiles or related affiliate tiles. We might need a different attribute of adtype={directory,enhanced,related}.

There's also a backwards compatibility issue of if we just reuse the v2 endpoint, old Firefox clients will fetch the tiles and not realize type="related" tiles should not be shown and end up showing them as directory tiles. This can be easily fixed by making Firefox request v3 instead of v2.

An alternative approach is to just have different keys for each type:

{ "related": [{"title": "tile", "type": "affiliate"},
  "directory": [{...},...] }

(I happen to drop the explicit locale as our experience with v2 is we only serve one locale's tile anyway.)

Comment 1

[Olivier Yiptong [:oyiptong]]

I tend to like the second approach better, i.e. the approach with separate keys for each type:

{ "related": [{"title": "tile", "type": "affiliate"},
  "directory": [{...},...] }

A reason is that it allows for new types to be added in a backward-compatible way.
Another reason is that in the first scenario, the ordering of the object in the array becomes semantically irrelevant.

As Ed suggests, a new version of the endpoint will be required. Another benefit of the key separation is that this would require less code changes (even if the code change would've been minimal) to maintain the versions going forward.

Comment 2

[Ed Lee :Mardak]

Having the separate keys for each adtype makes it so we don't actually need to introduce an adtype="directory" for each tile, so we can keep the type={sponsored,organic,affiliate} as we have right now.

Comment 3

[Tim Spurway [:tspurway]]

The only question I have is about multiple keys instead of locales in the distribution.  How will the existing client code handle this?  Will it take the *first* key and use that as the set of tiles to display?

Comment 4

[Tim Spurway [:tspurway]]

:oyiptong and I had a chat about this today and I think came up with a solid plan for the distribution format.  I will explain the design we settled on, then I will give reasons why:

- we adopt a v3 onyx endpoint 
- we exclude all suggested tiles from the v2 distribution (ie. only show "directory" tiles)

For v3:
- we stop using the locale as the key and instead use "suggested" and "directory" as above
- inside the tile for a "suggested" tile, we will have an additional attribute "sites", which is a JSON list of suggested sites for this tile

We concluded that having the v3 would be the cleanest and simplest way to implement the functionality.  Splice will need to create both v2 and v3 distributions for the time being.

Comment 5

[Olivier Yiptong [:oyiptong]]

We might get away with having only the new kind of distribution, since it is strictly a superset of the current distribution.

The code in the onyx v2 API could parse the JSON differently than it does today and preserve the interface.

Comment 6

[Ed Lee :Mardak]

We'll need to update v1/v2 endpoints to strip out the new data from bug 1126191 to avoid confusing old clients while using existing data migrated by bug 1144039.

Comment 7

[Marina Samuel [:emtwo]]

Olivier, would it be possible to get the finalized json format that will be distributed for v3 for bug 1143745? Thanks!

Comment 8

[Olivier Yiptong [:oyiptong]]

We're both waiting for tspurway to implement the ingest format so we can work on our bugs.

The ingestion and distribution formats are discussed in bug 1126191.

The new distribution will be passed on to onyx to be served to Firefox.

Comment 9

[Ed Lee :Mardak]

How we serve is separate from how we injest. Is there anything wrong with using what you proposed in comment 2 and tspurway agreed in comment 4? Get rid of the locale check and the top level object has keys of "suggested" and "directory" with suggested tiles having a new "sites" array of strings?

Is it okay for emtwo to write the Firefox code expecting this format?

{
    "suggested": [
        {
            "sites": ["play.google.com", "androidsomething.com", "yetanotherandroidsite.net"],
            "bgColor": "#ffffff",
            "directoryId": 499,
            "enhancedImageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "imageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "title": "Firefox for Android",
            "type": "affiliate",
            "url": "https://play.google.com/store/apps/..."
        },
        {}
    ],
    "directory": [
        {
            "bgColor": "",
            "directoryId": 498,
            "enhancedImageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "imageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "title": "Mozilla Community",
            "type": "affiliate",
            "url": "http://contribute.mozilla.org/"
        },
        {}
    ]
}

Comment 10

[Olivier Yiptong [:oyiptong]]

I think this is correct. From our meeting last week, the only difference from this format is that the "sites" key is now called "frecent_sites".

Your input looks like:

{
    "suggested": [
        {
            "frecent_sites": ["play.google.com", "androidsomething.com", "yetanotherandroidsite.net"],
            "bgColor": "#ffffff",
            "directoryId": 499,
            "enhancedImageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "imageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "title": "Firefox for Android",
            "type": "affiliate",
            "url": "https://play.google.com/store/apps/..."
        },
        {}
    ],
    "directory": [
        {
            "bgColor": "",
            "directoryId": 498,
            "enhancedImageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "imageURI": "https://dtex4kvbppovt.cloudfront.net/images/...",
            "title": "Mozilla Community",
            "type": "affiliate",
            "url": "http://contribute.mozilla.org/"
        },
        {}
    ]
}

emtwo can expect this input

Comment 11

[Ed Lee :Mardak]

Per bug 1148938, it would have been nice to be able to send test tiles to Nightly and Aurora users.

Can we make the v3 endpoint be..

https://tiles.services.mozilla.com/v2/links/fetch/%LOCALE%/%CHANNEL%

where the server doesn't do anything with the channel quite yet.

Comment 12

[Olivier Yiptong [:oyiptong]]

Good idea. I like that we get channel support. We can even start logging the channel before we have functionality

Comment 13

[Olivier Yiptong [:oyiptong]]

Benson suggests to make the version of onyx in development backward compatible with the older version of the tile index file to make for smoother deployment

Comment 14

[Tim Spurway [:tspurway]]

:oyiptong, would this be a change in splice to add a backward compatible index file?  or a change in onyx to detect either style of index file?

Comment 15

[Olivier Yiptong [:oyiptong]]

would be a change in onyx. essentially, it would detect the tile index format and choose to serve v2 only or to also serve v3

Comment 16

[[github robot]]

Commit pushed to master at https://github.com/mozilla/onyx

https://github.com/mozilla/onyx/commit/7a4d681a14b4a2dfcc143ceff7ed8e2afcf4ee75
Bug 1134550 - Serve suggested tiles with v3 endpoint while keeping v1/v2 compatibility

Squashed commit of the following:

commit f707c4cd38ba9700b5bf2ec7134a4fc6c084c6e6
Author: Olivier Yiptong <[e-mail]>
Date:   Wed Apr 22 16:26:43 2015 -0400

    remove compatibility for old index file in environment.py (keep testing coverage though)

commit 377bd03e53f55dc1b43c6dd9af9ef5be9c1d16a8
Author: Olivier Yiptong <[e-mail]>
Date:   Tue Apr 21 14:34:55 2015 -0400

    add backward compatibility for legacy tile indexes

commit e1b77c9b99c75d65606c0dc9d575f3debe5b8c6f
Author: Olivier Yiptong <[e-mail]>
Date:   Mon Apr 20 11:57:34 2015 -0400

    ignore wsgi.py module in coverage

commit 7db0d4a865b13629ad61199116d398f3993cf51f
Author: Olivier Yiptong <[e-mail]>
Date:   Mon Apr 20 11:57:10 2015 -0400

    remove unsuded encryption module

commit 2ea9ae1db8c91583018766993dd3433894014c20
Author: Olivier Yiptong <[e-mail]>
Date:   Mon Apr 20 11:50:29 2015 -0400

    PEP8 fix

commit 60906e4ba8b7e1e528669bd84c2851d7c3f9b07f
Author: Olivier Yiptong <[e-mail]>
Date:   Mon Apr 20 11:48:31 2015 -0400

    remove error checking for payload errors in v3. there are no payloads anymore

commit 66244f01397f3cd942daa61628ba462100fa16a3
Author: Olivier Yiptong <[e-mail]>
Date:   Mon Apr 20 11:40:38 2015 -0400

    initial v3 api

commit 710e718a17644618385c47e1349d351ce8d003c6
Author: Olivier Yiptong <[e-mail]>
Date:   Mon Apr 6 17:06:43 2015 -0400

    PEP8 adjustments

commit 05ed0b4a4a361f9816e2331cb32aed72e21ebd63
Author: Olivier Yiptong <[e-mail]>
Date:   Mon Apr 6 16:59:31 2015 -0400

    initial v3 api implementation and tests

closes bug 1134550
closes #15

Comment 17

[Ed Lee :Mardak]

This landed back on 24th.