Skip to content

Report active experiments in error logs#8108

Merged
jridgewell merged 12 commits intoampproject:masterfrom
jridgewell:active-experiment-error-reporting
Mar 27, 2017
Merged

Report active experiments in error logs#8108
jridgewell merged 12 commits intoampproject:masterfrom
jridgewell:active-experiment-error-reporting

Conversation

@jridgewell
Copy link
Copy Markdown
Contributor

Fixes #7484.

@dvoytenko dvoytenko requested review from lannka and removed request for dvoytenko March 14, 2017 00:28
@dvoytenko
Copy link
Copy Markdown
Contributor

/to @lannka given the related work recently.

src/error.js Outdated
url += `&jse=${detectedJsEngine}`;

const experiments = experimentTogglesOrNull();
url += `&exps=${JSON.stringify(experiments)}`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need encodeURIComponent. Would a more compact custom encoding make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also moved away from JSON to a custom format.

@erwinmombay
Copy link
Copy Markdown
Member

@ampprojectbot retry!

1 similar comment
@erwinmombay
Copy link
Copy Markdown
Member

@ampprojectbot retry!

@ampprojectbot
Copy link
Copy Markdown
Member

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to @erwinmombay @cramforce

  • build-system/global-configs/canary-config.json
  • build-system/global-configs/prod-config.json

/to @dvoytenko @jridgewell

  • src/chunk.js
  • src/error.js
  • src/experiments.js

/to @alanorozco @aghassemi @cvializ @chenshay @honeybadgerdontcare @dvoytenko @ericlindley-g @erwinmombay @Gregable @lannka @mrjoro @powdercloud @newmuis @jridgewell @kmh287 @cramforce @mkhatib @camelburrito @choumx @muxin @zhouyx

  • test/functional/test-error.js
  • test/functional/test-experiments.js

@jridgewell jridgewell force-pushed the active-experiment-error-reporting branch 2 times, most recently from a42f17b to 31f60dc Compare March 14, 2017 20:21
@jridgewell jridgewell force-pushed the active-experiment-error-reporting branch from ee54f30 to cadf19a Compare March 16, 2017 00:34
@ampprojectbot
Copy link
Copy Markdown
Member

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

  • tools/experiments/README.md

/to cramforce erwinmombay

  • build-system/global-configs/canary-config.json
  • build-system/global-configs/prod-config.json

/to dvoytenko jridgewell

  • src/chunk.js
  • src/error.js
  • src/experiments.js

/to aghassemi alanorozco camelburrito chenshay choumx cramforce cvializ dvoytenko ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx

  • test/functional/test-error.js
  • test/functional/test-experiments.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

@ampprojectbot
Copy link
Copy Markdown
Member

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

  • tools/experiments/README.md

/to cramforce erwinmombay

  • build-system/global-configs/canary-config.json
  • build-system/global-configs/prod-config.json

/to dvoytenko jridgewell

  • src/chunk.js
  • src/error.js
  • src/experiments.js

/to alanorozco camelburrito chenshay choumx cramforce cvializ dvoytenko ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx

  • test/_init_tests.js
  • test/functional/test-error.js
  • test/functional/test-experiments.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

if (win.AMP_CONFIG
&& Array.isArray(win.AMP_CONFIG['allow-url-opt-in'])
&& win.AMP_CONFIG['allow-url-opt-in'].length > 0) {
const allowed = win.AMP_CONFIG['allow-url-opt-in'];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't see how this is related to the PR description..
is it a separate thing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me to test what's actually enabled on the page, I need a centralized location of the possible url enabled experiments. Imagine a bunch of isExperimentOnAllowUrlOverride calls in the source, how would we know?

Not to mention that if we could figure something out, we would need a dependency on experiments.js in the error reporter, which I'd rather avoid.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

@jridgewell jridgewell force-pushed the active-experiment-error-reporting branch from d617eda to 531a324 Compare March 23, 2017 18:15
@ampprojectbot
Copy link
Copy Markdown
Member

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

  • tools/experiments/README.md

/to cramforce erwinmombay

  • build-system/global-configs/canary-config.json
  • build-system/global-configs/prod-config.json

/to dvoytenko jridgewell

  • src/error.js
  • src/experiments.js

/to alanorozco camelburrito chenshay choumx cvializ ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx

  • test/_init_tests.js
  • test/functional/test-error.js
  • test/functional/test-experiments.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

@jridgewell jridgewell force-pushed the active-experiment-error-reporting branch from cb30681 to 3c45442 Compare March 23, 2017 23:00
if (win.AMP_CONFIG
&& Array.isArray(win.AMP_CONFIG['allow-url-opt-in'])
&& win.AMP_CONFIG['allow-url-opt-in'].length > 0) {
const allowed = win.AMP_CONFIG['allow-url-opt-in'];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

@jridgewell jridgewell merged commit bc07d17 into ampproject:master Mar 27, 2017
@jridgewell jridgewell deleted the active-experiment-error-reporting branch March 27, 2017 17:00
lironzluf pushed a commit to lironzluf/amphtml that referenced this pull request Mar 28, 2017
* master: (34 commits)
  Prevent amp-carousel next/previous icons fade away on desktop (ampproject#8428)
  Turn on flag slidescroll-disable-css-snap” (ampproject#8436)
  Revert "temporarily turn off yarn (ampproject#8356)" (ampproject#8384)
  initial commit (ampproject#8404)
  Upgrades for Index Exchange amp-ad tags to report load statistics (ampproject#8054)
  amp-bind validation tweak (ampproject#8414)
  Fix an amp-instagram race condition (ampproject#8192)
  Use whitelist to restrict urlReplacement for scoped analytics element (ampproject#8360)
  Report active experiments in error logs (ampproject#8108)
  amp-bind: Catch exceptions in mutatedAttributesCallback (ampproject#8383)
  Fixing custom scroll-snap on IOS (ampproject#8391)
  Add experiment for using AmpContext class in integration.js (ampproject#8348)
  add (ampproject#8349)
  swipe api (ampproject#8357)
  skip 3 flaky tests (ampproject#8388)
  amp-bind: Expression complexity limit (ampproject#8321)
  add margin-bottom (ampproject#8350)
  Flying carpet: make container full viewport and center content (ampproject#8292)
  Service Registration: Document Click (ampproject#7882)
  Add a8ad (ampproject#8036)
  ...
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* Move URL experiment overrides into toggles

* Test that URL overrides cookies

* Add experiments to error reports

* Use custom format

* lint

* lint

* Fix tests

* Fix experiments hash

* Fix test

* Ensure AMP_EXP is reset when resetting toggles

* lint

* Move pump-early-frame into allow-url-opt-in
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants