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

Pass experiment channel via the AmpRuntimeTypeParam #24723

Merged
merged 2 commits into from Sep 26, 2019

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Sep 25, 2019

I named the param same as the experiment prefix.

One thing I found is that the param is only used for A4A, not related to inabox. So I left the experimentX-control alone.

We should also document all the new AMP_CONFIG.type somewhere.

to @erwinmombay @jeffkaufman

@jeffkaufman
Copy link
Contributor

One thing I found is that the param is only used for A4A, not related to inabox. So I left the experimentX-control alone.

Yes: on inabox the ad server chooses the runtime version and so already has that in logs. In A4A the ad server needs to be told how to categorize the ad.

@@ -846,6 +846,9 @@ export function getBinaryTypeNumericalCode(type) {
'control': '1',
'canary': '2',
'rc': '3',
'experimentA': '10',
'experimentB': '11',
'experimentC': '12',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be include mod and nomod while we're at it?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, lets go ahead and do this.

Copy link
Member

Choose a reason for hiding this comment

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

i'll take up the 4x range (i've been using the RC build so its 43)

Copy link
Contributor Author

@zhouyx zhouyx Sep 25, 2019

Choose a reason for hiding this comment

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

Happy to add that. @erwinmombay to confirm it's

mod: 43
nomod: 44

?

Copy link
Member

Choose a reason for hiding this comment

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

so they are all essentially 43 LOL. they all have different versions, because unlike the rtv experiments they are deployed individually. the way i distinguish them differently is their custom version mark which is the last digit in the rtv (usually defaults to 0).

i can make changes to the deployment if we really need different prefixes between esm/sp/mp

Copy link
Contributor Author

@zhouyx zhouyx Sep 25, 2019

Choose a reason for hiding this comment

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

LOL, I wonder how 43 is selected? Why not 42 ?😛

I don't think we need different prefixes. Also the number here doesn't need to match the prefix number. It's just an id to understand which version of runtime is used to served the A4A. As long as we agree on the id there, it should be fine. Especially that unlike exp_a/b/c, the mod/nomod experiment is temporary.

What about nomod: 42, and mod: 43 to match the style control (even): exp (odd). Let me know if every agrees on the id. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

yeah im ok with that. its 43 because i wanted to take the 4x range, and 3 cause rc is original 03 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤣🤣🤣
Added mod and nomod type. PTAL

@zhouyx zhouyx merged commit a6429e7 into ampproject:master Sep 26, 2019
@zhouyx zhouyx deleted the amp-runtime-param branch September 26, 2019 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants