Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added docs/ab-testing.md
Empty file.
25 changes: 21 additions & 4 deletions ilc/client/ClientRouter.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export default class ClientRouter extends EventEmitter {
#logger;
#registryConf;
#ilcConfigRoot;
#ilcState;
/** @type Object<Router> */
#router;
#prevRoute;
Expand Down Expand Up @@ -54,6 +55,7 @@ export default class ClientRouter extends EventEmitter {
this.#location = location;
this.#logger = logger;
this.#i18n = i18n;
this.#ilcState = state ?? {};
this.#ilcConfigRoot = ilcConfigRoot;
this.#registryConf = this.#ilcConfigRoot.getConfig();
this.#router = new Router(this.#registryConf);
Expand Down Expand Up @@ -142,12 +144,27 @@ export default class ClientRouter extends EventEmitter {
if (routeConfig === undefined) {
throw new this.errors.RouterError({ message: 'Can not find info about the slot.', data: { slotName } });
}
const appProps = appConfig.props || {};
// ILC-level props for the app/slot. `appConfig.props` is the app's registry
// config; the user-app props live under a nested `appProps` key (below).
const appConfigProps = appConfig.props || {};
const routeProps = routeConfig.props || {};

const finalRouteProps = deepmerge(appProps, routeProps);

return finalRouteProps;
// Carry the experiment assignments (resolved once at SSR and inlined into
// ilcState) into every app's props on client-side navigation. They go under the
// nested `appProps` key — the same place the server uses (server-router.js) and
// the field a client consumer reads via getCurrentPathProps().appProps —
// so a freshly client-mounted app still receives the already-resolved assignment.
// This reuses the existing value (never re-buckets), keeping the assignment stable
// across all apps in a long-lived SPA session.
//
// Note: SSR-rendered (primary) apps already get experiments via the server SSR
// appProps, so this client-side merge is the forward-looking half of that same
// contract — it is what delivers the assignment to apps mounted purely on the
// client (no SSR). It is a no-op when ilcState carries no experiments.
const { experiments } = this.#ilcState;
const experimentsProps = experiments ? { appProps: { experiments } } : {};

return deepmerge.all([appConfigProps, routeProps, experimentsProps]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a bit expensive operation. I do not see alternative now but it could be a bottleneck potentially

}

#setInitialRoutes = (state) => {
Expand Down
47 changes: 47 additions & 0 deletions ilc/client/ClientRouter.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,53 @@ describe('client router', () => {
'Can not find info about the slot',
);
});

it('does not add an appProps.experiments field when ilcState carries no experiments', () => {
// router from beforeEach is built with empty ilcState ({}), so props are untouched
chai.expect(router.getCurrentRouteProps('@portal/hero', 'hero').appProps).to.equal(undefined);
});

it('merges ilcState.experiments into appProps so client-side navigation carries the assignment', () => {
const experiments = { 'homepage-hero': 'variant-b', 'example-experiment': 'variant-a' };
// Rebuild the beforeEach router with experiments in ilcState. Reassign the
// module-level `router` (after cleaning the empty one) so afterEach tears down
// its window listeners — a local instance would leak handlers into later
// navigation specs and trigger a full-page reload.
router.removeEventListeners();
router = new ClientRouter(configRoot, { experiments }, undefined, singleSpa, handlePageTransaction);

// nested under `appProps` to match the server (server-router.js), so a client
// consumer can read it via getCurrentPathProps().appProps on mount
chai.expect(router.getCurrentRouteProps('@portal/hero', 'hero').appProps).to.be.eql({ experiments });
chai.expect(router.getPrevRouteProps('@portal/hero', 'hero').appProps).to.be.eql({ experiments });
});

it('preserves the app/route own props alongside the injected experiments', () => {
const experiments = { 'homepage-hero': 'variant-b' };
router.removeEventListeners();
router = new ClientRouter(configRoot, { experiments }, undefined, singleSpa, handlePageTransaction);

const props = router.getCurrentRouteProps('@portal/hero', 'hero');
// the app's own props must survive the merge — experiments are purely additive
chai.expect(props.heroSecondProp).to.equal('heroSecondProp');
chai.expect(props.heroFirstProp).to.be.an('object');
chai.expect(props.appProps).to.be.eql({ experiments });
});

it('reuses the resolved assignment without re-bucketing (stable across current/prev and repeated calls)', () => {
const experiments = { 'homepage-hero': 'variant-b' };
router.removeEventListeners();
router = new ClientRouter(configRoot, { experiments }, undefined, singleSpa, handlePageTransaction);

// same resolved value every time — the client never re-evaluates the assignment
chai.expect(router.getCurrentRouteProps('@portal/hero', 'hero').appProps.experiments).to.be.eql(
experiments,
);
chai.expect(router.getCurrentRouteProps('@portal/hero', 'hero').appProps.experiments).to.be.eql(
experiments,
);
chai.expect(router.getPrevRouteProps('@portal/hero', 'hero').appProps.experiments).to.be.eql(experiments);
});
});

describe('when i18n was provided', () => {
Expand Down
3 changes: 3 additions & 0 deletions ilc/config/custom-environment-variables.json5
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,8 @@
},
client: {
protocol: 'ILC_CLIENT_PROTOCOL',
},
experiments: {
enabled: 'EXPERIMENTS_ENABLED',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

prefix with ILC_? Not sure, but it looks like half of the variables follow this convention.

}
}
19 changes: 18 additions & 1 deletion ilc/config/default.json5
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,22 @@
},
client: {
protocol: 'https',
}
},
experiments: {
// Global kill-switch. Set to false (e.g. via env in an incident) to send every
// visitor to control with no assignment. Toggling this is a config change, not a deploy.
enabled: true,
// Experiment definitions, keyed by id. Empty in the OSS baseline: a deployment
// declares its own experiments in an environment/local config layer (see
// config/local.json5 for the local demo; the integration test injects its own
// ruleset from tests/fixtures/experiments.json5), so a fresh install assigns no
// one and writes no x-ab-* cookies until it opts in.
//
// This static-JSON ruleset is the current source, read by
// server/experiments/StaticConfigRulesetProvider. See docs/ab-testing.md for a
// later direction where a remote experiment-management service (management UI +
// SSE/polling delivery) becomes the source of truth; until then, experiments are
// configured here.
ruleset: {},
},
}
14 changes: 13 additions & 1 deletion ilc/server/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,20 @@ import { TransitionHooksExecutor } from './TransitionHooksExecutor';
const { Test500Error } = require('./errorHandler/ErrorHandler');

const i18n = require('./i18n');
const { applyExperiments } = require('./experiments');
const reportingPluginManager = require('./plugins/reportingPlugin');
const AccessLogger = require('./logger/accessLogger');
const { isStaticFile, isHealthCheck, isDataUri } = require('./utils/utils');

/**
* @param {Registry} registryService
* @param {*} pluginManager
* @param {import('./experiments').Ruleset} [experimentsRuleset] Optional ruleset override
* for the experiment layer. Left undefined in production so `applyExperiments` reads the
* config-loaded ruleset; the integration test passes a fixture here to drive the real
* onRequest path deterministically, without a node-config test layer.
*/
module.exports = async function createApplication(registryService, pluginManager) {
module.exports = async function createApplication(registryService, pluginManager, experimentsRuleset) {
const reportingPlugin = reportingPluginManager.getInstance();
const errorHandler = errorHandlerFactory();
const appConfig = Application.getConfig(reportingPlugin);
Expand Down Expand Up @@ -62,6 +68,12 @@ module.exports = async function createApplication(registryService, pluginManager
);

await i18nOnRequest(req, reply);

try {
applyExperiments(req, reply, experimentsRuleset);
} catch (error) {
logger.warn({ error }, 'Failed to apply experiment assignments; continuing with control');
}
});

app.addHook('onResponse', (req, reply, done) => {
Expand Down
49 changes: 47 additions & 2 deletions ilc/server/app.spec.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
const fs = require('fs');
const path = require('path');
const chai = require('chai');
const JSON5 = require('json5');
const nock = require('nock');
const supertest = require('supertest');
const helpers = require('../tests/helpers');
const createApp = require('./app');

async function createTestServer(mockRegistryOptions = {}, mockPluginOptions = {}) {
// Experiment ruleset for the integration test. Loaded explicitly here and injected via
// createApp's seam, rather than through a node-config test layer, so it stays scoped to
// this spec instead of leaking into every NODE_ENV=test file's config.
const experimentsRuleset = JSON5.parse(
fs.readFileSync(path.join(__dirname, '../tests/fixtures/experiments.json5'), 'utf8'),
).ruleset;

async function createTestServer(mockRegistryOptions = {}, mockPluginOptions = {}, rulesetOverride) {
const app = await createApp(
helpers.getRegistryMock(mockRegistryOptions),
helpers.getPluginManagerMock(mockPluginOptions),
rulesetOverride,
);

await app.ready();
Expand All @@ -26,7 +37,7 @@ describe('App', () => {

before(async () => {
helpers.setupMockServersForApps();
const serverInstance = await createTestServer();
const serverInstance = await createTestServer({}, {}, experimentsRuleset);
app = serverInstance.app;
server = serverInstance.server;
});
Expand Down Expand Up @@ -341,4 +352,38 @@ describe('App', () => {

chai.expect(routerProps.reqUrl).to.include('?foo=bar&test=123');
});

// Integration coverage for the experiment onRequest hook through the real Fastify
// stack (the ungated `example-experiment` ruleset is injected from
// tests/fixtures/experiments.json5 via createApp — see the top of this file).
describe('experiment assignment', () => {
const setCookies = (response) => response.headers['set-cookie'] ?? [];

it('mints a session cookie and assigns the ungated experiment over a real request', async () => {
const response = await server.get('/').expect(200);
const cookies = setCookies(response);

chai.expect(cookies.some((c) => c.startsWith('ilc-sid='))).to.equal(true);
const ab = cookies.map((c) => c.split(';')[0]).find((c) => c.startsWith('x-ab-example-experiment='));
chai.expect(ab, 'x-ab-example-experiment cookie set by the onRequest hook').to.exist;
// the resolved value is a real declared variant of the experiment
const variant = ab.split('=')[1];
chai.expect(['variant-a', 'variant-b']).to.include(variant);
// a personalized (variant-bearing) response must not be shared-cached
chai.expect(response.headers['cache-control']).to.equal('private, no-store');
});

it('reuses an existing assignment and does not re-issue its cookie', async () => {
const first = await server.get('/').expect(200);
const firstNames = setCookies(first).map((c) => c.split(';')[0]);
const sid = firstNames.find((c) => c.startsWith('ilc-sid='));
const abCookie = firstNames.find((c) => c.startsWith('x-ab-example-experiment='));
chai.expect(sid, 'session cookie issued on first visit').to.exist;
chai.expect(abCookie, 'assignment cookie issued on first visit').to.exist;

const second = await server.get('/').set('Cookie', `${sid}; ${abCookie}`).expect(200);
const reissued = setCookies(second).some((c) => c.startsWith('x-ab-example-experiment='));
chai.expect(reissued).to.equal(false);
});
});
});
54 changes: 54 additions & 0 deletions ilc/server/experiments/StaticConfigRulesetProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import config from 'config';
import type { Ruleset, RulesetProvider } from './interfaces';
import { validateRuleset } from './validate';

/**
* Current implementation of the experiment ruleset source: reads the ruleset from the
* static JSON config layer — node-config `experiments.ruleset` (empty in the OSS baseline;
* populated in a local/registry config layer, or injected in tests).
*
* This is the single place that knows where the ruleset comes from. The assignment layer
* (assign / bucket / consent / cookies / propagation) only ever receives a resolved
* `Ruleset` and never reads configuration, so the source can evolve on its own. See
* `docs/ab-testing.md` for a later direction where a remote experiment-management service
* — a management UI plus SSE/polling delivery — becomes the source of truth, letting
* stakeholders manage experiments without an ILC deployment. That would arrive as another
* {@link RulesetProvider} implementation
* swapped in at `./ruleset`, with no change to the assignment layer — which is the point
* of keeping the source behind this seam.
*
* The read is defensive: a malformed ruleset degrades to "no experiments" rather than
* crashing ILC bootstrap, and validation problems are surfaced. It runs at construction
* time — before the app's structured (pino) logger exists — so problems go to stdout.
*/
export class StaticConfigRulesetProvider implements RulesetProvider {
private readonly ruleset: Ruleset;

constructor() {
this.ruleset = StaticConfigRulesetProvider.read();
}

public getRuleset(): Ruleset {
return this.ruleset;
}

private static read(): Ruleset {
if (!config.has('experiments.ruleset')) {
return {};
}

try {
const raw = config.get<Ruleset>('experiments.ruleset');
const problems = validateRuleset(raw);
if (problems.length > 0) {
// eslint-disable-next-line no-console
console.warn(`[experiments] ruleset has issues:\n ${problems.join('\n ')}`);
}
return raw;
} catch (error) {
// eslint-disable-next-line no-console
console.warn('[experiments] failed to load ruleset; running with no experiments', error);
return {};
}
}
}
Loading
Loading