Amphtml: 🚨 Error: amp-consent must have an amp-story-consent child

[ampprojectbot]

Details

Error report: link
First seen: Dec 24, 2019
Frequency: ~ 11687/day

Stacktrace

Error: amp-consent must have an amp-story-consent child
    at Error (https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/src/log.js:714)
    at apply (https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/src/log.js:308)
    at apply (https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/src/log.js:323)
    at error (https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/extensions/amp-story/1.0/amp-story.js:1179)
    at validateConsent_ (https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/extensions/amp-story/1.0/amp-story.js:1150)
    at handleConsentExtension_ (https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/extensions/amp-story/1.0/amp-story.js:963)

@rsimha modified src/log.js:308-311 in #21212 (May 16, 2019)
@erwinmombay modified src/log.js:323-324 in #6581 (Dec 19, 2016)
@gmajoulet modified extensions/amp-story/1.0/amp-story.js:1178-1183 in #15155 (May 8, 2018)
@gmajoulet modified extensions/amp-story/1.0/amp-story.js:1149-1158 in #15916 (Jun 14, 2018)
@rsimha modified extensions/amp-story/1.0/amp-story.js:962-964 in #21212 (May 16, 2019)

Potential assignees: @gmajoulet

[rcebulko]

It looks like this may be a misconfiguration on the part of the user, if there's a missing tag. Should this be filed as a user() error?

[rsimha]

Can we do away with @mentioning the person who last changed a line? The PR linked in this report changed > 100k lines and has nothing to do with the error.

This workflow will also spam the same person for all call stacks that end in an async throw, for example.

A less noisy approach may be to put the author’s name inside backticks.

[gmajoulet]

This is a publisher misconfiguration. Are dev().error() statements supposed to exist in production? I thought they were removed from the build.

cc @ampproject/wg-stories

[rcebulko]

@gmajoulet This is an unfortunately common misunderstanding. devAssert() statements are removed, but dev().error() statements report an error. user.error() statements report a "user error" indicating something that isn't broken in AMP but is misused/misconfigured in the publisher's page; these are handled separately from dev errors.

@rsimha I'm experimenting with using git blame to detect and tag the users who most likely are relevant to a given error, with the goal of eventually allowing a monitoring system to flag significant errors and automatically bring them to the attention of the team via GitHub issues. Currently it mentions up to three users who have edited the lines in the stack trace most recently (within the past two years).

This is manually triggered by me and experimentally it was working pretty well, with this caveat appearing. Since you added Prettier and modified almost every file and line in the repo, that unfortunately added noise to the git blame history. I'm investigating how to possibly work around this case, including by narrowing the change window based on the "First seen" date and by detecting/blacklisting your Prettier PRs from consideration. Disabling @ mentions in the interim.

[gmajoulet]

Got it, thanks for clarifying. So changing this dev().error() to user().error() would be the right fix here?

[rcebulko]

Updated this comment to reflect the changes; @ mentions are now in backticks and the comment includes a suggestion for who to assign, but doesn't actually tag anyone.