Add EditionIdCAPIMapper
#26830
Add EditionIdCAPIMapper
#26830
Conversation
ioannakok
force-pushed
the
fix-most-popular-capi-query-for-europe-and-int-edition
branch
from
4051d8d
to
730a4d7
Compare
| @@ -82,8 +81,8 @@ object Edition { | |||
| } | |||
|
|
|||
| def apply(request: RequestHeader): Edition = { | |||
| val cookieValue = editionFromRequest(request) | |||
There was a problem hiding this comment.
Renaming because cookieValue is misleading. This method returns the edition from a parameter, a header or a cookie.
| @@ -76,7 +76,7 @@ trait ApiQueryDefaults extends GuLogging { | |||
|
|
|||
| def item(id: String): ItemQuery = CapiContentApiClient.item(id) | |||
|
|
|||
| def item(id: String, edition: Edition): ItemQuery = item(id, edition.id) | |||
| def item(id: String, edition: Edition): ItemQuery = item(id, EditionIdCAPIMapper.mapEditionId(edition)) | |||
There was a problem hiding this comment.
The mapping looks good to me as Content-api's Conceirge reads us, uk, au, europe, international so it has to be this to give the content.
Mapping done in this PR should be fine. Still will get CoPip team to give review on this as well.
Just being curious, some que on this:
-
Can we try changing Edition id from
EURtoEUROPE? -
If not above, then can we try accessing networkfrontId here as that refers to
europe, so can we try checking if edition.id != edition.networkfrontId (which is true in this case) so then accessnetworkFrontIdinstead? (I know here priority should beedition.idbut just curious to know if we can do these cases, then we might not need mapping.
There was a problem hiding this comment.
Thank you so much @Divs-B for the review! Re point 2, networkFrontId achieves indeed what we want without having to map. However, I would prefer to avoid this solution as it’s more subtle and it wouldn't be super clear from its name why we're passing the network id instead of the id. Whereas at least the EditionIdCAPIMapper is more descriptive. I agree though the ideal solution would be option 1, to change the id. Unfortunately this requires a big refactoring in at least 3 repos from what I can tell. Haven't done full analysis but here are some examples:
- frontend -->
edition.idis passed in many cases and we will have to check downstream and upstream - https://github.com/guardian/dotcom-rendering --> DCR model uses
EURandINT - https://github.com/guardian/fastly-edge-cache --> We'll have to make sure that changing the IDs works well with our Fastly CDN code
For these reasons we would prefer to go with the mapper for now but I have created a ticket to rename the IDs as this is the best approach:
And one question: Is Concierge the interface to all CAPI requests? Does that mean that all of CAPI's endpoints expect the long form id i.e. europe, international?
There was a problem hiding this comment.
Thanks @ioannakok for your answers.
Details about the work it requires to refactor in many places, I think it does help in justifying to go ahead with the mapping at the moment in this PR as a best solution.
Indeed we would want to keep a consistent name (example "EUROPE") as id across all Guardian repos hence #26851 would be a great idea. Thanks for that, Ioanna.
We are happy to approve this.
There was a problem hiding this comment.
Regarding your query:
Is Concierge the interface to all CAPI requests? Does that mean that all of CAPI's endpoints expect the long form id i.e. europe, international?
Yes
(but I will confirm with my team as well) 👍
There was a problem hiding this comment.
Team also has confirmed the same for Concierge 👍
As aforementioned, consistent name would be great idea to keep and as long as Concierge reading correct form of id it should return the content whether any long form in it. Thankyou.
There was a problem hiding this comment.
Hi @Divs-B thanks for your review!
Out of an abundance of caution we checked in Kibana whether any other requests (other than most-viewed) are made to CAPI from frontend using the short edition ids of EUR or INT. Using the query in the Content Platforms space and logstash-capi-* index we could see a variety of requests using EUR or INT
Here are few example queries to demonstrate the different frontend applications making requests to CAPI using the short form ids of either EUR or INT:
EUR form the facia app
INT from the article app
INT from applications
INT from applications-crosswords
Manually updating the edition id to the long form id returns the same response so we assume the edition parameter does not actually affect the query response.
Could you please confirm our understanding that the queries above do not use the edition parameter so updating the edition id to the long form should have no effect.
Thanks!
There was a problem hiding this comment.
Hello. CAPI only becomes interested in the edition parameter if the query is for what looks like a network front (from days before the Fronts API existed IIRC) or an editionalised section, e.g. sport. In the examples you've linked, these appear to be mostly individual content item or tag searches, and those will ignore the edition. If you're curious about which sections are editionalised, you can go to content.guardianapis.com/sections and look for any with multiple editions listed. Hope this helps 🤞
There was a problem hiding this comment.
Thanks @JustinPinner and @Divs-B very helpful
We noticed that the `MostPopularAgent` queries CAPI using the wrong edition id for the Europe and International editions. It uses "EUR" and "INT" instead of "EUROPE" and "INTERNATIONAL" as expected by CAPI. We're introducing this mapper that will populate the agent with content for these two editions. Co-authored-by: Ravi <[email protected]>
ioannakok
force-pushed
the
fix-most-popular-capi-query-for-europe-and-int-edition
branch
from
730a4d7
to
31ddec2
Compare
ioannakok
deleted the
fix-most-popular-capi-query-for-europe-and-int-edition
branch
ABSOLUTELY. I BELIEVE THAT'S THE WAY TO GO IN PR DESCRIPTIONS AS WELL |
|
Seen on FRONTS-PROD (merged by @ioannakok 11 minutes and 14 seconds ago)
|
|
Seen on ADMIN-PROD (merged by @ioannakok 11 minutes and 25 seconds ago)
|
Closes #9553
Paired with @arelra
What does this change?
Introduces a mapper that passes "EUROPE" instead of "EUR" and "INTERNATIONAL" instead of "INT" in the item queries to CAPI.
Why?
MostPopularAgentqueries CAPI using the wrong edition id for the Europe and International editions. It uses "EUR" and "INT" instead of "EUROPE" and "INTERNATIONAL" as expected by CAPI. As a result, the following endpoints called by DCRMostViewedFooterDatareturn a 404:The change we're making populates the agent with content for these two editions and fixes the above endpoints.
Note
We think that the best solution for this problem is to rename the ids for the Europe and International edition but this
needs more analysis and we expect it to be a bigger change that will touch frontend, DCR and fastly-edge-cache.
See comment here. We've created a ticket.
Screenshots
Checklist