-
Notifications
You must be signed in to change notification settings - Fork 560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Prebid Bidder Performance in Admin Frontend tools #27506
Conversation
559aa67
to
f09358e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think we can also now get rid of the dateLineCharts.scala.html
file as it looks like only the TeamKPIController was referencing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
dev-build/conf/routes
Outdated
@@ -274,7 +274,6 @@ GET /commercial/non-refreshable-line-items.json | |||
|
|||
GET /ads.txt commercial.controllers.AdsDotTextViewController.renderTextFile() | |||
GET /app-ads.txt commercial.controllers.AdsDotTextViewController.renderAppTextFile() | |||
GET /commercial/prebid/revenue controllers.admin.commercial.TeamKPIController.renderPrebidDashboard() | |||
POST /commercial/api/hb commercial.controllers.PrebidAnalyticsController.insert() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icydk there's also the /hb
endpoint which powers this graph.
Of which there is a router, prebid and data platform aspect:
- https://github.com/guardian/platform/blob/362efa1eb6e5b4bbdbeecb48a9146abb5941a553/router/files/router.conf#L406
- https://github.com/guardian/Prebid.js/blob/9671a714e8b8a0229ca4f9581fb4150c8ba065b3/modules/guAnalyticsAdapter.js#L135
But we can follow up seperately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am leaning towards removing the /hb
endpoint from routes and dev-build routes and all related things to PrebidAnalyticsController
and like you said the other two aspects could be a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue with removing /hb
in this PR is that we'd need to remove the call to /hb
from our prebid fork first so the call does not error in the clients. Probably better to remove from prebid first and then from Frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that in Prebid and have a ready PR to merge then merge this PR after so I don't have to create two PRs in Frontend. What I am trying to understand right now the Platform repo router conf has to be removed when during the process I mentioned, any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thats fine as long you don't mind putting this PR on hold. I think Router changes changes can go anytime after we stop sending the /hb
request from the client.
So first:
prebid (no more requests to /hb
)
Then as there are no more requests to /hb
we can do an the following in any order:
router
Frontend
Ah yes we can do that! Thanks for pointing it out 👍🏼 |
9371ff4
to
73c4cd3
Compare
Seen on ADMIN-PROD (created by @deedeeh and merged by @emma-imber 12 minutes and 1 second ago)
|
Seen on FRONTS-PROD (created by @deedeeh and merged by @emma-imber 14 minutes and 11 seconds ago)
|
What does this change?
This PR removes the Prebid Bidder Performance from Performance section in Commercial tools which is found in admin Frontend tools.
The Commercial team is tidying up the Commercial tools in admin page and this is the first step towards it.
Related PRs guardian/Prebid.js#159 and https://github.com/guardian/platform/pull/1625
The merging order for the PRs as follows #27506 (comment)
Screenshots
Checklist