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
Fast Fetch: change logging of non-AMP render from error to dev warn #13108
Fast Fetch: change logging of non-AMP render from error to dev warn #13108
Conversation
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
this.promiseErrorHandler_( | ||
new Error('fallback to 3p'), | ||
/* ignoreStack */ true); | ||
dev().warn(TAG, 'fallback to 3p'); |
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 feel like this is such a common case that it's almost always just going to be noise. Can we maybe downgrade it to fine
so that it doesn't show up in the dev tools by default?
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 would want to AMP core to signoff on that first. It may be a useful signal for them. For now I want to make sure it does not confuse publishers as a false indicator for incorrectly configuring the ad tag.
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.
By "useful signal", do you mean you want it to phone home to the logging server? I guess we can do warn
for now as a kind of hacky way of doing that, but it would be better to send that using a separate mechanism so that we're not polluting the logs. Maybe add a TODO for this?
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.
fine
will completely remove the message from the production binary.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
@@ -1282,6 +1282,7 @@ export class AmpA4A extends AMP.BaseElement { | |||
'fallback to 3p disabled'); | |||
return Promise.resolve(false); | |||
} | |||
// TODO(keithwrightbos): remove when no longer needed. |
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 linter might complain about the absence of an issue number here, but if it doesn't care, I don't care.
…mpproject#13108) * Fast Fetch: change logging of non-AMP render from error to dev warn * PR feedback
…mpproject#13108) * Fast Fetch: change logging of non-AMP render from error to dev warn * PR feedback
…mpproject#13108) * Fast Fetch: change logging of non-AMP render from error to dev warn * PR feedback
…mpproject#13108) * Fast Fetch: change logging of non-AMP render from error to dev warn * PR feedback
…mpproject#13108) * Fast Fetch: change logging of non-AMP render from error to dev warn * PR feedback
Current message is available in console leading to confusion around incorrect page tagging.