[BUG FIX] [MER-5885] Restore UpGrade data capture parity for native A/B experiments - #6789
Conversation
AI Review — elixirNo issues found |
AI Review — performanceNo issues found |
AI Review — uiNo issues found |
AI Review — typescriptNo issues found |
AI Review — securityNo issues found |
e019915 to
d66de5f
Compare
99219bd to
2854b3b
Compare
66dace9 to
2bbe5da
Compare
d66de5f to
59c9003
Compare
| FROM raw_events | ||
| WHERE section_id = {section_id:UInt64} | ||
| AND event_type = 'activity_attempt' | ||
| AND verb_id = 'http://adlnet.gov/expapi/verbs/evaluated' |
There was a problem hiding this comment.
I think this query cannot currently reconstruct the compatibility dataset from the activity events we emit. It filters verb_id = '.../evaluated', but ActivityAttemptEvaluated.new/3 emits .../completed and both direct/Lambda project that as activity_attempt.
The producer test currently asserts the enrollment extension but not the emitted verb, while the compatibility-query test uses hand-built raw-event fixtures whose default verb is .../evaluated. Could we make these paths share one contract—align the query with the emitted verb and build the compatibility fixture from ActivityAttemptEvaluated.new/3 (or assert the same canonical verb in both tests)—so this cannot drift again?
| ) AS raw | ||
| ASOF LEFT JOIN | ||
| ( | ||
| SELECT |
There was a problem hiding this comment.
If I understand correctly, a section may have active experiments at different Decision Points. This compatibility query only takes section_id as scope, unlike the query cookbook (docs/user/experiments/queries.md), which scopes experiment analysis by experiment_id. Could we either add an experiment_id parameter here or explicitly document that the query picks the most recent assignment for an enrollment regardless of experiment? Otherwise a researcher could silently attach a condition from a different concurrent experiment to an outcome.
| end) | ||
| end | ||
|
|
||
| defp hydrate_assignment_match(match) do |
There was a problem hiding this comment.
hydrate_assignment_match/1 here is nearly identical to the one in media_attributions.ex, apart from section_slug. Would it make sense to extract the common hydration step so the two projections cannot drift the next time their selected fields change?
| ) | ||
|
|
||
| result | ||
| case result do |
There was a problem hiding this comment.
Small consistency note, not a bug: the single-assignment path receives {:ok, decision} for sticky reuse and therefore never calls its emission callback, while the batch path reaches ConditionAssignmentEmitter for the same case and relies on its reused?: true clause to return :ok. Both paths are covered and correct today. Would it be worth converging on one place to own that guard when this flow is next touched?
https://eliterate.atlassian.net/browse/MER-5885
Summary
Restores the experiment analytics data needed for UpGrade-style research and analysis while keeping raw learner outcomes independent from optional causal attribution.
Experiment evidence
reward_valueremainsNULL, while explicit values including0.0are preservedInitial condition assignments
experiment_condition_assignedxAPI statement and raw-event typeassigned_atvalue and links the attribution to the dedicated statement hashRaw analytics and ClickHouse parity
enrollment_idacross applicable attempt, page-view, tutor-message, and video statementsraw_events.user_idwithout falling back to actor email/mbox valuesraw_event_typefor clarity before production releaseResearcher and operator experience
Important design boundaries
Verification
mix compilemix format --check-formattedgit diff --checkReconciliation status
1c14fc71c9539b3c98d5hotfix-v0.34.1git range-diffconfirmed all retained MER-5885 commits were patch-equivalent