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
Add new amp-analytics vendor: New Relic #11464
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
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.
LGTM with one request.
}, | ||
'vars': { | ||
'beacon': 'bam.nr-data.net', | ||
'appId': [''], |
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.
nit: []
is good enough.
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.
Updated. Thanks!
42f969d
to
1a781c4
Compare
@zhouyx Thanks for the quick review! I'm just waiting on the legal time to give me the thumbs up on signing that doc. |
1a781c4
to
bb1ee61
Compare
@blainesch Did your company sign the CLA? If so please reply your company name to this thread after the doc is signed. Thanks. |
bb1ee61
to
e63a875
Compare
I signed it! |
@blainesch You sign the CLA as an individual contributor right? If that's the case googlebot should be able to pick that up fast.
If so my best suggestion would be to rebase, it sometimes fix the CLA. |
@zhouyx I signed it as an employer. I created this commit to have my work email address, which is part of the google group I signed the doc with. Edit: |
e63a875
to
2dbe1e5
Compare
I signed it! |
2dbe1e5
to
11cb1c6
Compare
@blainesch If your company signed the CLA, that may take longer. Can you provide the name of your company and we can see if it has been processed or not. |
@zhouyx it's |
@blainesch I am not able to locate New Relic, Inc on the list. Please note that it typically takes a few days to process the corporate CLA. It you think it's taking longer than expected, please reach out to check status. Thanks! |
I'll check back later next week. Thank you! |
@zhouyx any update on the cla? |
Hmmm I am still not able to find the company name. Just sent out an email to check the status, I'll let you know when I hear back from them. Thanks |
@blainesch Some update: It appears that New Relic corporate CLA is still under review because it has not yet been signed by the New Relic's representative. |
I saw the form being filled out, are you sure? I can ask them to resubmit it... |
Oh, it emailed them and they thought it was spam. They found it and signed it from the email. |
@blainesch Good News! New Relic Inc now appears in the accepted corporate CLAs list. Please reply to this thread to enable googlebot to verify : ) |
I signed it! |
CLAs look good, thanks! |
@blainesch Thanks for the Pull Request! Merged |
@zhouyx thanks for helping me! I have another question if you have time. I tried to use the snippet and I'm getting an error. You can see an example in this repo: https://github.com/blainesch/amp/blob/short-amp-snippet/analytics-short.html#L27-L37 Here's a screenshot of the error I'm getting: Any advice on why this isn't working would be appreciated. Thank you! |
Hey @blainesch. You are testing against our PROD version which doesn't have your vendor config included yet. That's why amp-analytics can't find a request string. This PR was merged on Thursday, didn't make to this week's canary cut. Unfortunately you can only test it locally right now. After next week's canary cut, your config will be available by opt into our dev channel . The change is expected to roll out to prod one week after canary (target at around 11/15) |
Thank you [: |
This adds New Relic to the amp-analytics provider list.
Fixes #11463