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: adds datasource status to sdk-client #590

Merged
merged 23 commits into from
Sep 30, 2024
Merged
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
60aae7c
feat: adds datasource status to sdk-client
tanderson-ld Sep 23, 2024
7ef3b5c
self review comments
tanderson-ld Sep 23, 2024
6ef2c49
lint issues
tanderson-ld Sep 23, 2024
2849c1f
Merge remote-tracking branch 'origin' into ta/sc-249239/data-source-s…
tanderson-ld Sep 24, 2024
6f8c6b5
fixing missing identify resolve
tanderson-ld Sep 24, 2024
ff6dda5
removing unused StoreError
tanderson-ld Sep 24, 2024
c4879f5
updated datasource states. shutdown to be closed and now closed and …
tanderson-ld Sep 25, 2024
d95111f
making change detection tests a little less sensitive
tanderson-ld Sep 25, 2024
1c584d3
Merge remote-tracking branch 'origin' into ta/sc-249239/data-source-s…
tanderson-ld Sep 26, 2024
6028c89
now passing emitter to DataSourceStatusManager and fixing related tests
tanderson-ld Sep 26, 2024
90bbc69
linting
tanderson-ld Sep 26, 2024
a2787a6
fixing lint and incorrect commenting in test
tanderson-ld Sep 26, 2024
ae3f473
fixing missed merge logic and fixing unit tests
tanderson-ld Sep 26, 2024
1c24e1b
remove unused import
tanderson-ld Sep 26, 2024
8dd7590
Merge branch 'main' into ta/sc-249239/data-source-status-rebase
tanderson-ld Sep 26, 2024
47b6b70
fixing imports wip
tanderson-ld Sep 27, 2024
ddeb130
Fix build.
kinyoklion Sep 30, 2024
60ce6c5
Fix common tests.
kinyoklion Sep 30, 2024
7df3547
Merge branch 'main' into ta/sc-249239/data-source-status-rebase
kinyoklion Sep 30, 2024
15adf36
tidy
kinyoklion Sep 30, 2024
11e430c
Browser initializing from poll.
kinyoklion Sep 30, 2024
df41f81
Add error handling for single poll.
kinyoklion Sep 30, 2024
57796af
Documentation and string enum.
kinyoklion Sep 30, 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
27 changes: 0 additions & 27 deletions packages/shared/common/src/errors.ts
Original file line number Diff line number Diff line change
@@ -2,33 +2,6 @@
// more complex, then they could be independent files.
/* eslint-disable max-classes-per-file */

export class LDFileDataSourceError extends Error {
constructor(message: string) {
super(message);
this.name = 'LaunchDarklyFileDataSourceError';
}
}

export class LDPollingError extends Error {
public readonly status?: number;

constructor(message: string, status?: number) {
super(message);
this.status = status;
this.name = 'LaunchDarklyPollingError';
}
}

export class LDStreamingError extends Error {
public readonly code?: number;

constructor(message: string, code?: number) {
super(message);
this.code = code;
this.name = 'LaunchDarklyStreamingError';
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: These moved to datasource/errors.ts

export class LDUnexpectedResponseError extends Error {
constructor(message: string) {
super(message);
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export enum DataSourceErrorKind {
/// An unexpected error, such as an uncaught exception, further
/// described by the error message.
Unknown,

/// An I/O error such as a dropped connection.
NetworkError,

/// The LaunchDarkly service returned an HTTP response with an error
/// status, available in the status code.
ErrorResponse,

/// The SDK received malformed data from the LaunchDarkly service.
InvalidData,
}
39 changes: 39 additions & 0 deletions packages/shared/common/src/internal/datasource/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/* eslint-disable max-classes-per-file */
import { DataSourceErrorKind } from './DataSourceErrorKinds';

export class LDFileDataSourceError extends Error {
Copy link
Member

Choose a reason for hiding this comment

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

Errors are not intended to be internal. In that the internal package should only be used by SDK implementations, but instance of LDPollingError theoretically is something you could do in an application. Maybe we don't want that, but this one is maybe breaking.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe not expected to be constructed outside though. That part is likely fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Started fixing this, but running into some import/export configuration issue. Pushed most recent commit before having to leave for flight.

constructor(message: string) {
super(message);
this.name = 'LaunchDarklyFileDataSourceError';
}
}

export class LDPollingError extends Error {
public readonly kind: DataSourceErrorKind;
public readonly status?: number;
public readonly recoverable: boolean;

constructor(kind: DataSourceErrorKind, message: string, status?: number, recoverable = true) {
super(message);
this.kind = kind;
this.status = status;
this.name = 'LaunchDarklyPollingError';
this.recoverable = recoverable;
}
}

export class LDStreamingError extends Error {
public readonly kind: DataSourceErrorKind;
public readonly code?: number;
public readonly recoverable: boolean;

constructor(kind: DataSourceErrorKind, message: string, code?: number, recoverable = true) {
super(message);
this.kind = kind;
this.code = code;
this.name = 'LaunchDarklyStreamingError';
this.recoverable = recoverable;
}
}

export type StreamingErrorHandler = (err: LDStreamingError) => void;
15 changes: 15 additions & 0 deletions packages/shared/common/src/internal/datasource/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { DataSourceErrorKind } from './DataSourceErrorKinds';
import {
LDFileDataSourceError,
LDPollingError,
LDStreamingError,
StreamingErrorHandler,
} from './errors';

export {
DataSourceErrorKind,
LDFileDataSourceError,
LDPollingError,
LDStreamingError,
StreamingErrorHandler,
};
3 changes: 2 additions & 1 deletion packages/shared/common/src/internal/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export * from './context';
export * from './datasource';
export * from './diagnostics';
export * from './evaluation';
export * from './events';
export * from './stream';
export * from './context';
Original file line number Diff line number Diff line change
@@ -2,8 +2,9 @@ import { createBasicPlatform, createLogger } from '@launchdarkly/private-js-mock

import { EventName, Info, LDLogger, ProcessStreamResponse } from '../../api';
import { LDStreamProcessor } from '../../api/subsystem';
import { LDStreamingError } from '../../errors';
import { defaultHeaders } from '../../utils';
import { DataSourceErrorKind } from '../datasource/DataSourceErrorKinds';
import { LDStreamingError } from '../datasource/errors';
import { DiagnosticsManager } from '../diagnostics';
import StreamingProcessor from './StreamingProcessor';

@@ -260,7 +261,7 @@ describe('given a stream processor with mock event source', () => {

expect(willRetry).toBeFalsy();
expect(mockErrorHandler).toBeCalledWith(
new LDStreamingError(testError.message, testError.status),
new LDStreamingError(DataSourceErrorKind.Unknown, testError.message, testError.status),
);
expect(logger.error).toBeCalledWith(
expect.stringMatching(new RegExp(`${status}.*permanently`)),
Original file line number Diff line number Diff line change
@@ -7,12 +7,12 @@ import {
Requests,
} from '../../api';
import { LDStreamProcessor } from '../../api/subsystem';
import { LDStreamingError } from '../../errors';
import { ClientContext } from '../../options';
import { getStreamingUri } from '../../options/ServiceEndpoints';
import { httpErrorMessage, LDHeaders, shouldRetry } from '../../utils';
import { DataSourceErrorKind } from '../datasource/DataSourceErrorKinds';
import { LDStreamingError, StreamingErrorHandler } from '../datasource/errors';
import { DiagnosticsManager } from '../diagnostics';
import { StreamingErrorHandler } from './types';

const reportJsonError = (
type: string,
@@ -22,7 +22,9 @@ const reportJsonError = (
) => {
logger?.error(`Stream received invalid data in "${type}" message`);
logger?.debug(`Invalid JSON follows: ${data}`);
errorHandler?.(new LDStreamingError('Malformed JSON data in event stream'));
errorHandler?.(
new LDStreamingError(DataSourceErrorKind.InvalidData, 'Malformed JSON data in event stream'),
);
};

// TODO: SDK-156 - Move to Server SDK specific location
@@ -87,7 +89,9 @@ class StreamingProcessor implements LDStreamProcessor {
private retryAndHandleError(err: HttpErrorResponse) {
if (!shouldRetry(err)) {
this.logConnectionResult(false);
this.errorHandler?.(new LDStreamingError(err.message, err.status));
this.errorHandler?.(
new LDStreamingError(DataSourceErrorKind.ErrorResponse, err.message, err.status),
);
this.logger?.error(httpErrorMessage(err, 'streaming request'));
return false;
}
@@ -142,7 +146,12 @@ class StreamingProcessor implements LDStreamProcessor {
}
processJson(dataJson);
} else {
this.errorHandler?.(new LDStreamingError('Unexpected payload from event stream'));
this.errorHandler?.(
new LDStreamingError(
DataSourceErrorKind.Unknown,
'Unexpected payload from event stream',
),
);
}
});
});
2 changes: 0 additions & 2 deletions packages/shared/common/src/internal/stream/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import StreamingProcessor from './StreamingProcessor';
import type { StreamingErrorHandler } from './types';

export { StreamingProcessor };
export type { StreamingErrorHandler };
3 changes: 0 additions & 3 deletions packages/shared/common/src/internal/stream/types.ts

This file was deleted.

11 changes: 3 additions & 8 deletions packages/shared/mocks/src/streamingProcessor.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import type {
ClientContext,
EventName,
internal,
LDHeaders,
LDStreamingError,
ProcessStreamResponse,
} from '@common';
import type { ClientContext, EventName, internal, LDHeaders, ProcessStreamResponse } from '@common';

type LDStreamingError = internal.LDStreamingError;

export const MockStreamingProcessor = jest.fn();

48 changes: 20 additions & 28 deletions packages/shared/sdk-client/__tests__/LDClientImpl.storage.test.ts
Original file line number Diff line number Diff line change
@@ -87,11 +87,8 @@ describe('sdk-client storage', () => {

expect(mockPlatform.storage.get).toHaveBeenCalledWith(flagStorageKey);

// 'change' should not have been emitted
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenNthCalledWith(
2,
expect(emitter.emit).toHaveBeenCalledWith('change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenCalledWith(
'error',
context,
expect.objectContaining({ message: 'test-error' }),
@@ -140,15 +137,12 @@ describe('sdk-client storage', () => {
);

// 'change' should not have been emitted
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(
1,
expect(emitter.emit).toHaveBeenCalledWith(
'change',
expect.objectContaining(toMulti(context)),
defaultFlagKeys,
);
expect(emitter.emit).toHaveBeenNthCalledWith(
2,
expect(emitter.emit).toHaveBeenCalledWith(
'error',
expect.objectContaining(toMulti(context)),
expect.objectContaining({ message: 'test-error' }),
@@ -175,15 +169,17 @@ describe('sdk-client storage', () => {

// @ts-ignore
emitter = ldc.emitter;
jest.spyOn(emitter as LDEmitter, 'emit');
const spy = jest.spyOn(emitter as LDEmitter, 'emit');

// expect emission
await ldc.identify(context);
expect(emitter.emit).toHaveBeenCalledWith('change', context, defaultFlagKeys);

// expit no emission
// clear the spy so we can tell if change was invoked again
spy.mockClear();
// expect no emission
await ldc.identify(context);

expect(emitter.emit).toHaveBeenCalledTimes(1);
expect(emitter.emit).not.toHaveBeenCalledWith('change', context, defaultFlagKeys);
});

test('no storage, cold start from streaming', async () => {
@@ -256,8 +252,8 @@ describe('sdk-client storage', () => {
JSON.stringify(putResponse),
);

expect(emitter.emit).toHaveBeenNthCalledWith(1, 'change', context, defaultFlagKeys);
Copy link
Member

Choose a reason for hiding this comment

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

I may need some explanation on these changes also, as the ordering of events mattered and that isn't tested anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that too. I can go through and get them all to be passing, but I don’t think there is any specification around when a change event should come out vs when a dataSourceStatus change should come out and that is leading to them all breaking as I added dataSourceStatus events.

Perhaps an improved way could be to filter to just “change” and then do positional checking on the filtered output.

expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledWith('change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenCalledWith('change', context, ['dev-test-flag']);
});

test('syncing storage when a flag is added', async () => {
@@ -296,7 +292,7 @@ describe('sdk-client storage', () => {
flagStorageKey,
JSON.stringify(putResponse),
);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['another-dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledWith('change', context, ['another-dev-test-flag']);
});

test('syncing storage when a flag is updated', async () => {
@@ -319,7 +315,7 @@ describe('sdk-client storage', () => {
await jest.runAllTimersAsync();

expect(ldc.allFlags()).toMatchObject({ 'dev-test-flag': false });
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledWith('change', context, ['dev-test-flag']);
});

test('syncing storage on multiple flag operations', async () => {
@@ -347,7 +343,7 @@ describe('sdk-client storage', () => {

expect(ldc.allFlags()).toMatchObject({ 'dev-test-flag': false, 'another-dev-test-flag': true });
expect(ldc.allFlags()).not.toHaveProperty('moonshot-demo');
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, [
expect(emitter.emit).toHaveBeenCalledWith('change', context, [
'moonshot-demo',
'dev-test-flag',
'another-dev-test-flag',
@@ -380,8 +376,7 @@ describe('sdk-client storage', () => {
);

// we expect one change from the local storage init, but no further change from the PUT
expect(emitter.emit).toHaveBeenCalledTimes(1);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenCalledWith('change', context, defaultFlagKeys);

// this is defaultPutResponse
expect(ldc.allFlags()).toEqual({
@@ -423,7 +418,7 @@ describe('sdk-client storage', () => {

// both previous and current are true but inExperiment has changed
// so a change event should be emitted
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledWith('change', context, ['dev-test-flag']);
});

test('patch should emit change event', async () => {
@@ -452,8 +447,7 @@ describe('sdk-client storage', () => {
expect(ldc.allFlags()).toMatchObject({ 'dev-test-flag': false });
expect(mockPlatform.storage.set).toHaveBeenCalledTimes(4);
expect(flagsInStorage['dev-test-flag'].version).toEqual(patchResponse.version);
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledWith('change', context, ['dev-test-flag']);
});

test('patch should add new flags', async () => {
@@ -484,8 +478,7 @@ describe('sdk-client storage', () => {
expect.stringContaining(JSON.stringify(patchResponse)),
);
expect(flagsInStorage).toHaveProperty('another-dev-test-flag');
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['another-dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledWith('change', context, ['another-dev-test-flag']);
});

test('patch should ignore older version', async () => {
@@ -557,8 +550,7 @@ describe('sdk-client storage', () => {
expect.stringContaining('dev-test-flag'),
);
expect(flagsInStorage['dev-test-flag']).toMatchObject({ ...deleteResponse, deleted: true });
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledWith('change', context, ['dev-test-flag']);
});

test('delete should not delete equal version', async () => {
Loading