-
Notifications
You must be signed in to change notification settings - Fork 33
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
Experience sampling weight #80
Experience sampling weight #80
Conversation
Needs somebody from @Betterment/test_track_core to claim domain review Use the shovel operator to claim, e.g.:
|
json.feature_gate split.feature_gate? | ||
end | ||
end | ||
json.merge!({}) |
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.
This seems a little ugly but it was the only way I found to force the splits
key to appear with an empty object as a value. If the loop is empty, otherwise the split key disappears entirely. Is there a more revealing approach?
@split_registry.splits.each do |split| | ||
json.set! split.name do | ||
json.weights split.registry | ||
json.feature_gate split.feature_gate? |
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.
Another advantage of this representation would be that we could theoretically set different sample rates for different splits if e.g. different apps were running at different orders of magnitude of scale.
7c04ddc
to
b16a4ba
Compare
increase brittleness if env var retrieval impl changes
Document EXPERIENCE_SAMPLING_WEIGHT
b16a4ba
to
a2145bd
Compare
I'm in favor of the new split registry format, it should compress well since there are so many repeated strings, so the actual payload size might not change significantly. This all looks great! << domainlgtm @samandmoore can you take the other review? |
Needs somebody from @aburgel and @samandmoore to claim platform review Use the shovel operator to claim, e.g.:
|
Sure! I will give it a look tonight.
…On Fri, Jun 8, 2018 at 17:25 nanda-prbot ***@***.***> wrote:
Needs somebody from @aburgel <https://github.com/aburgel> and @samandmoore
<https://github.com/samandmoore> to claim platform review
Use the shovel operator to claim, e.g.:
@Myname << domain && platform
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAY4Itcbh46qN7OyHACBfDBUVYc1d4PGks5t6uvUgaJpZM4UdHnM>
.
|
Needs somebody from @aburgel and @samandmoore to claim platform review Use the shovel operator to claim, e.g.:
|
spec/models/split_registry_spec.rb
Outdated
end | ||
|
||
it "returns 1 with no env var" do | ||
with_env EXPERIENCE_SAMPLININGG_WEIGHT: nil do |
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.
Typo
<<platform |
Needs @samandmoore to provide platform review When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:
If you're too busy to review, unclaim the PR, 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.
Looks good! Just fix that typo.
platformlgtm |
Approved! 🔩 ⚡ 💫 |
Only downside to the compressibility is that presently when we pass the registry through to the JS client in the HTML payload we base64 it first, which will lose all the compressibility. I think the next gen answer here will probably involve GZipping the content before base64ing it or something. |
Summary
Add an EXPERIENCE_SAMPLING_WEIGHT env var and a new v2
split_registry
endpoint to expose its value to clients.The idea here is to tell clients to probablistically sample FeatureGateExperienced events at a given sampling weight. Sampling weight is the reciprocal of sample fraction, i.e. a sampling weight of 10 means that one in ten experience events should be reported to the analytics server and hence each sampled event represents ten real-world experiences.
This is a data volume reduction/cost saving measure. The default value is 1, meaning every experience event should be reported. The value 0 is intended to disable all experience events. This relies on the client reading the new split registry endpoint and sampling appropriately.
In order to make this feature work, I had to change the structure of the split_registries endpoint because it didn't have enough flexibility in it. It used to be just a JSON object with keys representing splits. I added a wrapper object and dropped the previous object into a
splits
key. That created space for aexperience_sampling_weight
key.While I was in there, I actually added more flex to the way split weightings are expressed: there's a similar issue there where the value of a given split was previously just a set of weightings. This means we can't transmit metadata abouta split other than weights in the v1 format. So I'm auditioning an expansion that puts the weightings in a
weights
key and allows us to add things like afeature_gate
boolean to the payload.This will add significant verbosity to the page. I'm not sure it's worth it, but it would've allowed me to stop having clients heuristically decide whether a given split is a feature gate or not based on suffix in the last iteration. Thoughts? I can pull that commit out of this PR if we don't want to go there.
/domain @Betterment/test_track_core
/platform @aburgel @samandmoore