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.
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.
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.
jmileham
force-pushed
the
experience_sampling_weight
branch
from
7c04ddc
to
b16a4ba
Compare
increase brittleness if env var retrieval impl changes
Document EXPERIENCE_SAMPLING_WEIGHT
jmileham
force-pushed
the
experience_sampling_weight
branch
from
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 |
|
<<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.:
|
|
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_registryendpoint 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
splitskey. That created space for aexperience_sampling_weightkey.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
weightskey and allows us to add things like afeature_gateboolean 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