Skip to content
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

feat: prevent fetching toggles when they are up to date #202

Closed
wants to merge 43 commits into from
Closed
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
7d231b0
to remove: change main file
jeremiewtd Apr 30, 2024
2fdf1e0
save last update key after fetch
jeremiewtd Apr 30, 2024
4f42346
refactor to handle 304 status
jeremiewtd May 2, 2024
ce45d89
unit test last update flag
jeremiewtd May 2, 2024
bab5000
handling isUpToDate
jeremiewtd May 2, 2024
a181bd5
update test
jeremiewtd May 2, 2024
c9a1677
fix existing test not working
Florent-Wanteeed May 3, 2024
3683bed
Store lastRefreshTimestamp to prevent async
Florent-Wanteeed May 3, 2024
195c438
fix toggleStorageTTL unit
Florent-Wanteeed May 3, 2024
ebcea49
refactor: rename option
jeremiewtd May 3, 2024
2f46c6b
unit test togglesStorage initial fetch
jeremiewtd May 3, 2024
ca8ebb9
refactor: emit ready method
jeremiewtd May 3, 2024
9463475
unit test togglesStorage ready
jeremiewtd May 3, 2024
9f6a10c
unit test ready not sent with bootstrap option
jeremiewtd May 3, 2024
cee9f51
unit test fetch when expired
jeremiewtd May 3, 2024
95f126c
documentation
jeremiewtd May 3, 2024
5ace0d9
improve readme
jeremiewtd May 6, 2024
c2f34e2
restore package.json
jeremiewtd May 6, 2024
cf12da6
revert public method
jeremiewtd May 6, 2024
63d22bf
restore package
jeremiewtd May 6, 2024
8c921af
readme
jeremiewtd May 6, 2024
c225d21
fix: linting
jeremiewtd May 6, 2024
54d5ab5
Merge remote-tracking branch 'origin/main' into feature/prevent-fetch…
jeremiewtd May 7, 2024
ad853c2
fix: lint
jeremiewtd May 7, 2024
44aa7f1
Merge branch 'main' into feature/prevent-fetch-on-load
jeremiewtd May 10, 2024
2d264a6
refactor: update timestamp key
jeremiewtd May 10, 2024
1556900
test: failing test that cover the bug
jeremiewtd May 10, 2024
bff8282
fix: can be up to date even with no flags
jeremiewtd May 10, 2024
b595afc
refactor: create initialFetchToggles to be used on start only
jeremiewtd May 10, 2024
0fc23d1
doc: update readme
jeremiewtd May 10, 2024
5bb8c20
fix: lint
jeremiewtd May 16, 2024
5266140
experimental and hash
Florent-Wanteeed Jun 10, 2024
7712d92
retour PR
Florent-Wanteeed Jun 12, 2024
4e7a656
Merge branch 'main' into feature/prevent-fetch
Florent-Wanteeed Jun 12, 2024
7684319
rollback previous test optim
Florent-Wanteeed Jun 12, 2024
9ecd585
Object Hash Value
Florent-Wanteeed Jun 26, 2024
8aec66c
Merge branch 'main' into feature/prevent-fetch
Florent-Wanteeed Jun 26, 2024
3c20789
system time goes back into the past
Florent-Wanteeed Jul 2, 2024
c0078f8
refacto
Florent-Wanteeed Jul 2, 2024
9e07183
Met a jour le flag fetchedFromServer quand on est a jour
Florent-Wanteeed Jul 2, 2024
04cd36e
retour
Florent-Wanteeed Jul 9, 2024
2fae8e0
retour PR
Florent-Wanteeed Jul 10, 2024
0a5d88c
Merge pull request #2 from wanteeed/feature/prevent-fetch
Florent-Wanteeed Jul 10, 2024
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ The Unleash SDK takes the following options:
| customHeaders | no| `{}` | Additional headers to use when making HTTP requests to the Unleash proxy. In case of name collisions with the default headers, the `customHeaders` value will be used if it is not `null` or `undefined`. `customHeaders` values that are `null` or `undefined` will be ignored. |
| impressionDataAll | no| `false` | Allows you to trigger "impression" events for **all** `getToggle` and `getVariant` invocations. This is particularly useful for "disabled" feature toggles that are not visible to frontend SDKs. |
| environment | no | `default` | Sets the `environment` option of the [Unleash context](https://docs.getunleash.io/reference/unleash-context). This does **not** affect the SDK's [Unleash environment](https://docs.getunleash.io/reference/environments). |
| togglesStorageTTL | no | `0` | How long (Time To Live), in seconds, the toggles in storage are considered valid and should not be fetched again. If set to 0 will disable expiration checking and will be considered always expired. |
ivarconr marked this conversation as resolved.
Show resolved Hide resolved

### Listen for updates via the EventEmitter

Expand Down
188 changes: 187 additions & 1 deletion src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
IConfig,
IMutableContext,
IToggle,
InMemoryStorageProvider,
UnleashClient,
} from './index';
import { getTypeSafeRequest, getTypeSafeRequestUrl } from './test';
Expand Down Expand Up @@ -553,6 +554,77 @@ test('Should publish ready when initial fetch completed', (done) => {
});
});

describe('handling last update flag storage', () => {
let storageProvider: IStorageProvider;
let saveSpy: jest.SpyInstance;
class Store implements IStorageProvider {
public async save() {
return Promise.resolve();
}

public async get() {
return Promise.resolve([]);
}
}

beforeEach(() => {
storageProvider = new Store();
saveSpy = jest.spyOn(storageProvider, 'save');
});

test('Should store last update flag when fetch is successful', async () => {
const startTime = Date.now();
fetchMock.mockResponseOnce(JSON.stringify(data));

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider,
};

const client = new UnleashClient(config);
await client.start();
expect(saveSpy).toHaveBeenCalledWith('repoLastUpdateTimestamp', expect.any(Number));
expect(saveSpy.mock.lastCall?.at(1)).toBeGreaterThanOrEqual(startTime);
});

test('Should store last update flag when fetch is successful with 304 status', async () => {
const startTime = Date.now();
fetchMock.mockResponseOnce(JSON.stringify(data), { status: 304 });

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider,
};

const client = new UnleashClient(config);
await client.start();
expect(saveSpy).toHaveBeenCalledWith('repoLastUpdateTimestamp', expect.any(Number));
expect(saveSpy.mock.lastCall?.at(1)).toBeGreaterThanOrEqual(startTime);
});

test('Should not store last update flag when fetch is not successful', async () => {
fetchMock.mockResponseOnce('', { status: 500 });

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider,
};

const client = new UnleashClient(config);
await client.start();
expect(saveSpy).not.toHaveBeenCalledWith(
'lastUpdate',
expect.any(Number)
);
});
});

test('Should publish error when initial init fails', (done) => {
const givenError = 'Error';

Expand Down Expand Up @@ -1483,15 +1555,16 @@ test('Should emit impression events on getVariant calls when impressionData is f
});

test('Should publish ready only when the first fetch was successful', async () => {
expect.assertions(3);
Copy link
Member

Choose a reason for hiding this comment

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

its a bit confusing to see updates to existing tests that seems unrelated to the PR. Can you please elaborate on why you needed to modify this scenario?

Copy link
Member

Choose a reason for hiding this comment

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

(the change looks reasonable tbh, but still would love to see your comments on them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an early version of our implementation, we needed to update this test and actually we realized that the test was not working as it should so we fixed it, but we can remove that if you think that should not be part of this pull request.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for a clarification. I would prefer to have it as a separate PR if you do not mind. We can keep it here as well

fetchMock.mockResponse(JSON.stringify(data));
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
refreshInterval: 1,
disableMetrics: true,
};
const client = new UnleashClient(config);
await client.start();

let readyCount = 0;

Expand All @@ -1503,6 +1576,8 @@ test('Should publish ready only when the first fetch was successful', async () =
expect(readyCount).toEqual(1);
});

await client.start();

jest.advanceTimersByTime(1001);
jest.advanceTimersByTime(1001);

Expand Down Expand Up @@ -1715,3 +1790,114 @@ describe('READY event emission', () => {
expect(client.emit).toHaveBeenCalledWith(EVENTS.READY);
});
});

describe('handling togglesStorageTTL > 0', () => {
let storage: IStorageProvider;
let fakeNow: number;
beforeEach(async () => {
fakeNow = new Date('2024-01-01').getTime();
jest.useFakeTimers();
jest.setSystemTime(fakeNow);
storage = new InMemoryStorageProvider();

fetchMock.mockResponseOnce(JSON.stringify(data)).mockResponseOnce(
JSON.stringify({
toggles: [
{
name: 'simpleToggle',
enabled: false,
impressionData: true,
},
],
})
);

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
};
// performing an initial fetch to populate the toggles and lastUpdate timestamp
const client = new UnleashClient(config);
await client.start();
client.stop();

expect(fetchMock).toHaveBeenCalledTimes(1);
fetchMock.mockClear();
});

afterEach(() => {
jest.useRealTimers();
});

test('Should not perform an initial fetch when toggles are up to date', async () => {
jest.setSystemTime(fakeNow + 59000);
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
togglesStorageTTL: 60,
};
const client = new UnleashClient(config);
await client.start();
const isEnabled = client.isEnabled('simpleToggle');
expect(isEnabled).toBe(true);
client.stop();
expect(fetchMock).toHaveBeenCalledTimes(0);
});

test('Should perform an initial fetch when toggles are expired', async () => {
jest.setSystemTime(fakeNow + 61000);

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
togglesStorageTTL: 60,
};
const client = new UnleashClient(config);
await client.start();
const isEnabled = client.isEnabled('simpleToggle');
expect(isEnabled).toBe(false);
client.stop();
expect(fetchMock).toHaveBeenCalledTimes(1);
});

test('Should send ready event when toggles are up to date', async () => {
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
togglesStorageTTL: 60,
};
const client = new UnleashClient(config);

const readySpy = jest.fn();
client.on(EVENTS.READY, readySpy);
client.on(EVENTS.INIT, () => readySpy.mockClear());
await client.start();
expect(readySpy).toHaveBeenCalledTimes(1);
});

test('Should perform a fetch when context is updated, even if flags are up to date', async () => {
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
togglesStorageTTL: 60,
};
const client = new UnleashClient(config);
await client.start();
let isEnabled = client.isEnabled('simpleToggle');
expect(isEnabled).toBe(true);
await client.updateContext({ userId: '123' });
expect(fetchMock).toHaveBeenCalledTimes(1);
isEnabled = client.isEnabled('simpleToggle');
expect(isEnabled).toBe(false);
});
});
71 changes: 56 additions & 15 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ interface IConfig extends IStaticContext {
customHeaders?: Record<string, string>;
impressionDataAll?: boolean;
usePOSTrequests?: boolean;
togglesStorageTTL?: number;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding it directly to the config definition, If we could have an optional "experimental" container we could make it clear that this option might go away or be changed in the future.

experimental?: {
  togglesStorageTTL?: number;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok we are working on it and adding the hashed context. Do you have any proposal for how you would like this to be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello

Concerning the hashed context, we started with the use of this type of library : “object-hash”.
Is this OK for you ?

unleash-proxy-client” lib size => 5.5 KB
object-hash” lib size => 10 KB

Which is a significant increase in percentage terms.

Last lib update: 02/2022 (v3.0.0),

Otherwise we can simply use a JSON.stringify (more verbose but which does not depend on anything)

Copy link
Member

Choose a reason for hiding this comment

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

We do not want to take on a library of this size for this.

I think the JSON.stringify is a good start, and as this is an experimental feature we can use that to learn the performance impact here. In the future we can consider to take advantage of the built in hasing capabilities, supported by most modern browsers (https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest)

}

interface IVariant {
Expand Down Expand Up @@ -93,6 +94,7 @@ const defaultVariant: IVariant = {
feature_enabled: false,
};
const storeKey = 'repo';
const lastUpdateKey = 'repoLastUpdateTimestamp';

type SdkState = 'initializing' | 'healthy' | 'error';

Expand Down Expand Up @@ -146,6 +148,8 @@ export class UnleashClient extends TinyEmitter {
private usePOSTrequests = false;
private started = false;
private sdkState: SdkState;
private togglesStorageTTL: number;
private lastRefreshTimestamp: number;

constructor({
storageProvider,
Expand All @@ -167,6 +171,7 @@ export class UnleashClient extends TinyEmitter {
customHeaders = {},
impressionDataAll = false,
usePOSTrequests = false,
togglesStorageTTL = 0,
}: IConfig) {
super();
// Validations
Expand Down Expand Up @@ -195,6 +200,9 @@ export class UnleashClient extends TinyEmitter {
this.context = { appName, environment, ...context };
this.usePOSTrequests = usePOSTrequests;
this.sdkState = 'initializing';
this.togglesStorageTTL = togglesStorageTTL * 1000;
this.lastRefreshTimestamp = 0;

this.ready = new Promise((resolve) => {
this.init()
.then(resolve)
Expand Down Expand Up @@ -348,6 +356,7 @@ export class UnleashClient extends TinyEmitter {
this.context = { sessionId, ...this.context };

this.toggles = (await this.storage.get(storeKey)) || [];
this.lastRefreshTimestamp = await this.storage.get(lastUpdateKey);

if (
this.bootstrap &&
Expand All @@ -374,7 +383,7 @@ export class UnleashClient extends TinyEmitter {
this.metrics.start();
const interval = this.refreshInterval;

await this.fetchToggles();
await this.initialFetchToggles();

if (interval > 0) {
this.timerRef = setInterval(() => this.fetchToggles(), interval);
Expand Down Expand Up @@ -426,6 +435,31 @@ export class UnleashClient extends TinyEmitter {
await this.storage.save(storeKey, toggles);
}

private isUpToDate() {
if (this.togglesStorageTTL <= 0) {
return false;
}
const timestamp = Date.now();

return !!(
this.lastRefreshTimestamp &&
timestamp - this.lastRefreshTimestamp <= this.togglesStorageTTL
);
}

private async updateLastRefreshTimestamp() {
this.lastRefreshTimestamp = Date.now();
this.storage.save(lastUpdateKey, this.lastRefreshTimestamp);
}

private initialFetchToggles() {
if (this.isUpToDate()) {
this.emitReady();
return;
}
return this.fetchToggles();
}

private async fetchToggles() {
if (this.fetch) {
if (this.abortController) {
Expand Down Expand Up @@ -460,20 +494,7 @@ export class UnleashClient extends TinyEmitter {
this.emit(EVENTS.RECOVERED);
}

if (response.ok && response.status !== 304) {
this.etag = response.headers.get('ETag') || '';
const data = await response.json();
await this.storeToggles(data.toggles);

if (this.sdkState !== 'healthy') {
this.sdkState = 'healthy';
}

if (!this.readyEventEmitted) {
this.emit(EVENTS.READY);
this.readyEventEmitted = true;
}
} else if (!response.ok && response.status !== 304) {
if (!response.ok && response.status !== 304) {
console.error(
'Unleash: Fetching feature toggles did not have an ok response'
);
Expand All @@ -483,6 +504,19 @@ export class UnleashClient extends TinyEmitter {
code: response.status,
});
}
else if (response.status !== 304) {
this.etag = response.headers.get('ETag') || '';
const data = await response.json();
await this.storeToggles(data.toggles);

if (this.sdkState !== 'healthy') {
this.sdkState = 'healthy';
}

this.emitReady();
}

this.updateLastRefreshTimestamp();
} catch (e) {
console.error('Unleash: unable to fetch feature toggles', e);
this.sdkState = 'error';
Expand All @@ -492,6 +526,13 @@ export class UnleashClient extends TinyEmitter {
}
}
}

private emitReady() {
jeremiewtd marked this conversation as resolved.
Show resolved Hide resolved
if (!this.readyEventEmitted) {
this.emit(EVENTS.READY);
this.readyEventEmitted = true;
}
}
}

// export storage providers from root module
Expand Down