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

Failed to process trigger "ad-request-start" #18231

Closed
adelinamart opened this issue Sep 20, 2018 · 5 comments · Fixed by #18340
Closed

Failed to process trigger "ad-request-start" #18231

adelinamart opened this issue Sep 20, 2018 · 5 comments · Fixed by #18340

Comments

@adelinamart
Copy link
Contributor

@lannka
Copy link
Contributor

lannka commented Sep 20, 2018

@calebcordry can you please take a look? It is from the AMP story analytics triggers.

@lannka lannka assigned calebcordry and unassigned lannka Sep 20, 2018
@calebcordry
Copy link
Member

Looking now.

@calebcordry
Copy link
Member

Upon further investigation it seems like this error is coming from

'triggers': {
'adRequestStart': csiTrigger('ad-request-start', {
// afs => ad fetch start
'met.a4a': 'afs_lvt.${viewerLastVisibleTime}~afs.${time}',
}),

Tagging a few people that were involved in the original PR @warrengm @keithwrightbos @glevitzky

Seems like the frequency has increased a bit recently. Bumping to p1 for now. The a4a team will be able to prioritize correctly.

@lannka
Copy link
Contributor

lannka commented Sep 20, 2018

It failed at here that this.analyticsGroup_ is null.

@zhouyx could you take a look if this is something to do with extension analytics.

@lannka lannka assigned zhouyx and unassigned calebcordry Sep 20, 2018
@zhouyx
Copy link
Contributor

zhouyx commented Sep 21, 2018

I've definitely seen the errors before. The reason is mostly because a race condition between the analytics initialization and parent element got remove from the page. We can verify it by introducing a new param, this.hasDetached_ and check on that before proceeding. Once we can verify that's the root cause, we can then remove the new param and simply check the existence of analytics group.

One thing I don't understand is why is related to story ad? @lannka Could you explain?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants