Skip to content

hot-fix: Matomo ab testing assignment distribution#15927

Merged
corwintines merged 2 commits into
masterfrom
hot-matomo-ab
Jul 25, 2025
Merged

hot-fix: Matomo ab testing assignment distribution#15927
corwintines merged 2 commits into
masterfrom
hot-matomo-ab

Conversation

@wackerow
Copy link
Copy Markdown
Member

@wackerow wackerow commented Jul 24, 2025

hot-patch to master

Description

  • use additional privacy-preserving header data to increase entropy of users when being assigned an AB test group
  • Uses improved hash function to help evenly distribute users based on fingerprint

Related Issue

AB Testing

wackerow added 2 commits July 24, 2025 12:07
use upgraded fingerprint with more elements to increase entropy, and use improved hash function to help evenly distribute users
@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 24, 2025

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 154255f
🔍 Latest deploy log https://app.netlify.com/projects/ethereumorg/deploys/68828b39ece67f000750deff
😎 Deploy Preview https://deploy-preview-15927--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 51 (🟢 up 1 from production)
Accessibility: 95 (🟢 up 1 from production)
Best Practices: 91 (🔴 down 1 from production)
SEO: 99 (no change from production)
PWA: 59 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added the tooling 🔧 Changes related to tooling of the project label Jul 24, 2025
@wackerow wackerow changed the title Hot matomo ab hot-fix: Matomo ab testing assignment distribution Jul 24, 2025
@minimalsm
Copy link
Copy Markdown
Contributor

@claude please review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jul 25, 2025

Claude finished @minimalsm's task —— View job


PR Review: Matomo A/B Testing Assignment Distribution

Reviewing the hot-fix for improved A/B test user distribution...

Todo Checklist:

  • Read and analyze app/api/ab-config/route.ts changes
  • Read and analyze src/lib/ab-testing/server.ts changes
  • Review the fingerprinting and hashing improvements
  • Check for security considerations
  • Evaluate code quality and best practices
  • Provide comprehensive review feedback

Review Summary ✅

This is a solid hot-fix that addresses A/B test distribution issues with well-implemented improvements. The changes enhance entropy for better user distribution while maintaining privacy and deterministic assignment.

Key Improvements

1. Enhanced Fingerprinting (src/lib/ab-testing/server.ts:36-46)

  • ✅ Good: Added accept-language and accept-encoding headers for increased entropy
  • ✅ Good: Included testKey in fingerprint to prevent cross-test correlation
  • ✅ Good: Changed from string concatenation to pipe-separated array join for better structure
  • ✅ Privacy-preserving: Uses standard HTTP headers that don't expose personal information

2. Improved Hash Function (src/lib/ab-testing/server.ts:73-82)

  • ✅ Excellent: Switched from djb2 to FNV-1a hash algorithm (better distribution properties)
  • ✅ Good: Proper 32-bit unsigned arithmetic with >>> 0 operator
  • ✅ Good: Correct normalization using full 32-bit range (0x100000000)
  • ✅ Good: Eliminated potential negative values from hash calculation

3. Environment Handling (app/api/ab-config/route.ts:36-37)

  • ✅ Good: Added IS_PROD check to ensure preview config in non-production environments
  • ✅ Good: Maintains existing IS_PREVIEW_DEPLOY behavior

Technical Analysis

Hash Distribution Quality: FNV-1a is a proven hash function with excellent avalanche properties, meaning small input changes result in dramatically different hash values. This is crucial for even A/B test distribution.

Entropy Sources: The additional headers provide meaningful entropy without compromising privacy:

  • accept-language: Varies by user locale/preferences
  • accept-encoding: Varies by browser capabilities
  • Combined with IP + User-Agent provides robust fingerprinting

Deterministic Consistency: Users will consistently receive the same variant across sessions due to stable header fingerprinting.

Security & Privacy

  • ✅ Privacy-compliant: No PII or tracking cookies used
  • ✅ GDPR-friendly: Uses only standard HTTP headers
  • ✅ No data persistence: Stateless assignment
  • ✅ Cross-test isolation: Test key inclusion prevents variant bleeding

Code Quality

  • ✅ Clean implementation: Well-commented and readable
  • ✅ Proper error handling: Maintains existing fallback behavior
  • ✅ Type safety: Maintains existing TypeScript contracts
  • ✅ Performance: Minimal computational overhead

Minor Suggestions

  1. Consider adding JSDoc for the hash function (assignVariantIndexDeterministic) to document the FNV-1a implementation
  2. Edge case documentation: The fingerprint fallback behavior could be documented (what happens with missing headers)

Overall Assessment

APPROVED

This hot-fix effectively addresses the A/B testing distribution issues with a mathematically sound approach. The FNV-1a hash implementation is correct, the additional entropy sources are appropriate, and the changes maintain backward compatibility while improving distribution quality.

The code is production-ready and aligns with the project's privacy-first approach to A/B testing.


@minimalsm
Copy link
Copy Markdown
Contributor

@coderabbitai please review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jul 25, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
# Ensure we have the latest main branch to diff against
git fetch origin main

echo "Changed files in this PR:"
git diff --name-status origin/main...HEAD

Length of output: 69


🏁 Script executed:

#!/bin/bash
set -e

# Check what remote branches are available
echo "Available remote branches:"
git branch -r

# Check current branch
echo -e "\nCurrent branch:"
git branch --show-current

# Try to get recent commits to understand the changes
echo -e "\nRecent commits:"
git log --oneline -10

# Get changed files using git status if this is a working directory
echo -e "\nGit status:"
git status --porcelain

Length of output: 331


🏁 Script executed:

#!/bin/bash
set -e

# Fetch dev branch
echo "Fetching origin/dev..."
git fetch origin dev

# Try to get the diff against dev
echo -e "\nChanged files compared to origin/dev:"
git diff --name-status origin/dev...HEAD

# If that doesn't work, let's see what files exist in the repo
echo -e "\nExploring repository structure:"
find . -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" -o -name "*.md" -o -name "*.json" | head -20

Length of output: 418


🏁 Script executed:

#!/bin/bash
set -e

PR_NUMBER=15927
PR_BRANCH="pr_${PR_NUMBER}"

echo "Fetching PR #$PR_NUMBER..."
git fetch origin pull/$PR_NUMBER/head:$PR_BRANCH

# List commits for sanity
echo -e "\nLast 5 commits in $PR_BRANCH:"
git log --oneline -5 $PR_BRANCH

# Show changed files vs dev
echo -e "\nChanged files compared to origin/dev:"
git diff --name-status origin/dev...$PR_BRANCH

Length of output: 511


🏁 Script executed:

#!/bin/bash
set -e

echo "Commit hash of pr_15927:"
git rev-parse pr_15927

echo -e "\nLast 10 commits on pr_15927:"
git log --oneline pr_15927 | head -10

echo -e "\nLast 10 commits on origin/dev:"
git log --oneline origin/dev | head -10

# Attempt to find merge base explicitly
echo -e "\nAttempting merge-base:"
git merge-base pr_15927 origin/dev || true

Length of output: 388


🏁 Script executed:

#!/bin/bash
set -e

# Check the specific commit
echo "Checking commit 154255f5743c07b23acc5847fb7bb82612f912d1:"
git show --name-only 154255f5743c07b23acc5847fb7bb82612f912d1

# List all files in current directory to understand repo structure
echo -e "\nFiles in repository root:"
ls -la

# Check if there are any staged/unstaged changes
echo -e "\nGit diff (staged):"
git diff --cached --name-status

echo -e "\nGit diff (working tree):"
git diff --name-status

Length of output: 266438


🏁 Script executed:

#!/bin/bash
set -e

# Get the actual changes in the commit with diff
echo "Actual changes in commit 154255f5743c07b23acc5847fb7bb82612f912d1:"
git show --stat 154255f5743c07b23acc5847fb7bb82612f912d1

echo -e "\n\nFull diff:"
git show 154255f5743c07b23acc5847fb7bb82612f912d1

Length of output: 262355


@minimalsm The PR appears to add the entire repository as new files, likely due to a missing or incorrect base branch. I recommend rebasing or recreating the PR from the correct base (e.g., dev) so I can provide a meaningful review of the intended changes. Let me know if you need guidance on how to do this!

@corwintines corwintines merged commit 54a0806 into master Jul 25, 2025
14 of 16 checks passed
@corwintines corwintines deleted the hot-matomo-ab branch July 25, 2025 15:30
This was referenced Jul 30, 2025
@konopkja konopkja mentioned this pull request Feb 12, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tooling 🔧 Changes related to tooling of the project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants