Skip to content
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

🌸 Cherry pick request for 26484 into 26356 (cherry-picked) #26483

Closed
powerivq opened this issue Jan 24, 2020 · 6 comments
Closed

🌸 Cherry pick request for 26484 into 26356 (cherry-picked) #26483

powerivq opened this issue Jan 24, 2020 · 6 comments
Assignees
Labels
Type: Release Used to track AMP releases from canary to production

Comments

@powerivq
Copy link
Contributor

powerivq commented Jan 24, 2020

Cherry-pick request

Issue PR Production? RC? Release issue
#26484 #26479 YES YES #26356

Why does this issue meet the cherry-pick criteria?

This is a production-affecting bug.

Why is a RC cherry-pick not needed?

Not applicable.

Mini-postmortem

TODO: This postmortem will be written after the cherry-pick deployment and before this issue is closed. Delete this TODO when the postmortem is ready.

Summary

We are refactoring several interfaces out from resources service to a mutator service. amp-auto-lightbox uses the API from resources service and was not switched to the new one. It was not caught until on production, and console error log triggered the alarm.

Users affected Impact
all users using amp-auto-lightbox on prod partial loss of functionality of amp-auto-lightbox

Root Causes

  1. amp-auto-lightbox uses element.getResources to obtain the service object, and the type checker is not able to find the type reference of that method. The type checker therefore failed to check whether the method call exists.

  2. There is insufficient traffic for this single extension on canary that would have triggered an alarm.

Action Items

Action Item Type Owner PR #
#26750 Prevent @choumx #<PR_NUMBER>

Lessons Learned

Things that went well

  • The refactoring itself was splitted into phases, so that the final switch-over PR was not large and easy to be reverted.

Things that went wrong

  • Traffic on canary is not large enough for relatively infrequent code paths.

/cc @ampproject/wg-approvers @ampproject/cherry-pick-approvers

@powerivq powerivq changed the title 🌸 Cherry pick request for <YOUR_ISSUE_NUMBER> into 26356 (Pending) 🌸 Cherry pick request for 26484 into 26356 (Pending) Jan 24, 2020
@powerivq
Copy link
Contributor Author

/cc @gmajoulet

@cramforce
Copy link
Member

Prod rolled back already right?

Is there a second cherry-pick coming? I thought that was the case.

@gmajoulet
Copy link
Contributor

The other cherry-pick is here: #26477

Both should be cherry-picked into prod before it is rollforwarded

@cramforce
Copy link
Member

Approved

@gmajoulet
Copy link
Contributor

We decided to not cherry-pick production and skip this week's production release.
Canary has been patched, is available as of a few minutes ago, and will be pushed to production next Tuesday.

Reassigning to @powerivq for the mini post mortem.

@gmajoulet gmajoulet assigned powerivq and unassigned cramforce Jan 25, 2020
@gmajoulet gmajoulet changed the title 🌸 Cherry pick request for 26484 into 26356 (Pending) 🌸 Cherry pick request for 26484 into 26356 (cherry-picked) Jan 25, 2020
@erwinmombay
Copy link
Member

closing this issue

@dreamofabear dreamofabear added the Type: Release Used to track AMP releases from canary to production label Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Release Used to track AMP releases from canary to production
Projects
None yet
Development

No branches or pull requests

5 participants