✨ Log custom analytics events in amp-video-iframe
#18869
Conversation
| @@ -247,7 +249,7 @@ class AmpVideoIframe extends AMP.BaseElement { | |||
| this.postIntersection_(messageId); | |||
| return; | |||
| } | |||
| dev().assert(false, `Unknown method '${methodReceived}`); | |||
There was a problem hiding this comment.
This message looks less useful without also logging the methodReceived variable. Was this to stop the "logging non-string" warning?
There was a problem hiding this comment.
Yup. I realized that assert takes var_args so the string could still be interpolated. Changing.
| analyticsEventType, '`eventType` missing in analytics event'); | ||
|
|
||
| user().assert( | ||
| analyticsEventType != VideoAnalyticsEvents.CUSTOM, |
There was a problem hiding this comment.
Why do you restrict this? Is there a danger of an infinite loop?
There was a problem hiding this comment.
Not infinite, but being caught twice and misreporting on the second call.
|
Is this a good candidate to place behind an experiment flag? It could give us a better chance to test and react to any bugs that could come up |
|
This pull request introduces 1 alert when merging f772e5c into 4e2a4f0 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
@cvializ For sure, what are the parts that you are concerned about that should be safeguarded? |
alanorozco
force-pushed
the
video-iframe-analytics
branch
from
03d9df9
to
4489d2f
Compare
|
I think the |
|
Is there existing documentation for this feature, or does it still need to be added? |
|
@cvializ No need to safeguard that since the entire component is experimental at this point :) |
|
@aghassemi Added unit tests. Adding to the integration tests will be done in a follow-up PR. |
…l into video-iframe-analytics
Borked this feature during review for #18869, as custom event names were introduced per request. Fixes by introducing an intermediate signal rather than overriding the event dispatched. VideoEventTracker for analytics only knows how to listen to a finite set of signals, so it's simpler to "redefine" the event type for user configuration on the analytics end. Fixes #22665
No description provided.