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

[EBT] Add Shipper "FullStory" #129927

Merged
merged 9 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions packages/elastic-analytics/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ NPM_MODULE_EXTRA_FILES = [
# "@npm//name-of-package"
# eg. "@npm//lodash"
RUNTIME_DEPS = [
"@npm//moment",
Copy link
Member

Choose a reason for hiding this comment

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

+1 for removing moment from the package and using plain old unix timestamps or Date objects

Copy link
Member Author

Choose a reason for hiding this comment

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

we need moment to validate the ISO string. We could replace it with a regex, but I worry we might leave out some formats depending on the TZ.

"@npm//rxjs",
]

Expand All @@ -51,6 +52,7 @@ RUNTIME_DEPS = [
TYPES_DEPS = [
"@npm//@types/node",
"@npm//@types/jest",
"@npm//moment",
"@npm//rxjs",
"//packages/kbn-logging:npm_module_types",
"//packages/kbn-logging-mocks:npm_module_types",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,44 @@ describe('AnalyticsClient', () => {
]);
});

test('The undefined values are not forwarded to the global context', async () => {
afharo marked this conversation as resolved.
Show resolved Hide resolved
const context$ = new Subject<{ a_field?: boolean; b_field: number }>();
analyticsClient.registerContextProvider({
name: 'contextProviderA',
schema: {
a_field: {
type: 'boolean',
_meta: {
description: 'a_field description',
optional: true,
},
},
b_field: {
type: 'long',
_meta: {
description: 'b_field description',
},
},
},
context$,
});

const globalContextPromise = globalContext$.pipe(take(6), toArray()).toPromise();
context$.next({ b_field: 1 });
context$.next({ a_field: false, b_field: 1 });
context$.next({ a_field: true, b_field: 1 });
context$.next({ b_field: 1 });
context$.next({ a_field: undefined, b_field: 2 });
await expect(globalContextPromise).resolves.toEqual([
{}, // Original empty state
{ b_field: 1 },
{ a_field: false, b_field: 1 },
{ a_field: true, b_field: 1 },
{ b_field: 1 }, // a_field is removed because the context provider removed it.
{ b_field: 2 }, // a_field is not forwarded because it is `undefined`
]);
});

test('Fails to register 2 context providers with the same name', () => {
analyticsClient.registerContextProvider({
name: 'contextProviderA',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,21 @@ export class ContextService {
[...this.contextProvidersRegistry.values()].reduce((acc, context) => {
return {
...acc,
...context,
...this.removeEmptyValues(context),
};
}, {} as Partial<EventContext>)
);
}

private removeEmptyValues(context?: Partial<EventContext>) {
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
if (!context) {
return {};
}
return Object.keys(context).reduce((acc, key) => {
if (context[key] !== undefined) {
acc[key] = context[key];
}
return acc;
}, {} as Partial<EventContext>);
}
}
35 changes: 35 additions & 0 deletions packages/elastic-analytics/src/events/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,42 @@

import type { ShipperName } from '../analytics_client';

/**
* Definition of the context that can be appended to the events through the {@link IAnalyticsClient.registerContextProvider}.
*/
export interface EventContext {
/**
* The unique user ID.
*/
userId?: string;
/**
* The user's organization ID.
*/
esOrgId?: string;
/**
* The product's version.
*/
version?: string;
/**
* The name of the current page.
* @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
*/
pageName?: string;
/**
* The current page.
* @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
*/
page?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Outside of the scope of this PR, but this makes me wonder: for the v3 telemetry shipper, will we have a specific field to dissociate events sent from the client and from the server?

Copy link
Member Author

@afharo afharo Apr 12, 2022

Choose a reason for hiding this comment

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

That's a good question! I haven't thought about it. If we want to, we can define different channel names when shipping the data to V3. I'll ask the Platform Analytics team in our next meeting today.

/**
* The current application ID.
* @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
*/
app_id?: string;
/**
* The current entity ID.
* @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
*/
ent_id?: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we define a good set of names for these fields and add a backwards-compatible layer in the FullStory shipper?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say so, yes. shipper-specific mappings aren't an issue, but I'd put that in the shipper implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated types to full names and shipper to maintain bwc

// TODO: Extend with known keys
[key: string]: unknown;
}
Expand Down
6 changes: 5 additions & 1 deletion packages/elastic-analytics/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ export type {
// Types for the registerEventType API
EventTypeOpts,
} from './analytics_client';

export type { Event, EventContext, EventType, TelemetryCounter } from './events';
export { TelemetryCounterType } from './events';

export type {
RootSchema,
SchemaObject,
Expand All @@ -52,4 +54,6 @@ export type {
AllowedSchemaStringTypes,
AllowedSchemaTypes,
} from './schema';
export type { IShipper } from './shippers';

export type { IShipper, FullStorySnippetConfig, FullStoryShipperConfig } from './shippers';
export { FullStoryShipper } from './shippers';
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import moment from 'moment';
import { formatPayload } from './format_payload';

describe('formatPayload', () => {
test('appends `_str` to string values', () => {
const payload = {
foo: 'bar',
baz: ['qux'],
};

expect(formatPayload(payload)).toEqual({
foo_str: payload.foo,
baz_strs: payload.baz,
});
});

test('appends `_int` to integer values', () => {
const payload = {
foo: 1,
baz: [100000],
};

expect(formatPayload(payload)).toEqual({
foo_int: payload.foo,
baz_ints: payload.baz,
});
});

test('appends `_real` to integer values', () => {
const payload = {
foo: 1.5,
baz: [100000.5],
};

expect(formatPayload(payload)).toEqual({
foo_real: payload.foo,
baz_reals: payload.baz,
});
});

test('appends `_bool` to booleans values', () => {
const payload = {
foo: true,
baz: [false],
};

expect(formatPayload(payload)).toEqual({
foo_bool: payload.foo,
baz_bools: payload.baz,
});
});

test('appends `_date` to Date values', () => {
const payload = {
foo: new Date(),
baz: [new Date()],
};

expect(formatPayload(payload)).toEqual({
foo_date: payload.foo,
baz_dates: payload.baz,
});
});

test('appends `_date` to moment values', () => {
const payload = {
foo: moment(),
baz: [moment()],
};

expect(formatPayload(payload)).toEqual({
foo_date: payload.foo,
baz_dates: payload.baz,
});
});

test('supports nested values', () => {
const payload = {
nested: {
foo: 'bar',
baz: ['qux'],
},
};

expect(formatPayload(payload)).toEqual({
nested: {
foo_str: payload.nested.foo,
baz_strs: payload.nested.baz,
},
});
});

test('does not mutate reserved keys', () => {
const payload = {
uid: 'uid',
displayName: 'displayName',
email: 'email',
acctId: 'acctId',
website: 'website',
pageName: 'pageName',
};

expect(formatPayload(payload)).toEqual(payload);
});

test('removes undefined values', () => {
const payload = {
foo: undefined,
baz: [undefined],
};

expect(formatPayload(payload)).toEqual({});
});

describe('String to Date identification', () => {
test('appends `_date` to ISO string values', () => {
const payload = {
foo: new Date().toISOString(),
baz: [new Date().toISOString()],
};

expect(formatPayload(payload)).toEqual({
foo_date: payload.foo,
baz_dates: payload.baz,
});
});

test('appends `_str` to random string values', () => {
const payload = {
foo: 'test-1',
baz: ['test-1'],
};

expect(formatPayload(payload)).toEqual({
foo_str: payload.foo,
baz_strs: payload.baz,
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import moment from 'moment';

// https://help.fullstory.com/hc/en-us/articles/360020623234#reserved-properties
const FULLSTORY_RESERVED_PROPERTIES = [
'uid',
'displayName',
'email',
'acctId',
'website',
// https://developer.fullstory.com/page-variables
'pageName',
];

export function formatPayload(context: Record<string, unknown>): Record<string, unknown> {
// format context keys as required for env vars, see docs: https://help.fullstory.com/hc/en-us/articles/360020623234
return Object.fromEntries(
Object.entries(context)
// Discard any undefined values
.map<[string, unknown]>(([key, value]) => {
return Array.isArray(value)
? [key, value.filter((v) => typeof v !== 'undefined')]
: [key, value];
})
.filter(
([, value]) => typeof value !== 'undefined' && (!Array.isArray(value) || value.length > 0)
)
// Transform key names according to the FullStory needs
.map(([key, value]) => {
if (FULLSTORY_RESERVED_PROPERTIES.includes(key)) {
return [key, value];
}
if (isRecord(value)) {
return [key, formatPayload(value)];
}
const valueType = getFullStoryType(value);
const formattedKey = valueType ? `${key}_${valueType}` : key;
return [formattedKey, value];
})
);
}

function getFullStoryType(value: unknown) {
// For arrays, make the decision based on the first element
const isArray = Array.isArray(value);
const v = isArray ? value[0] : value;
let type = '';
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
switch (typeof v) {
case 'string':
if (moment(v, moment.ISO_8601, true).isValid()) {
type = 'date';
break;
}
type = 'str';
break;
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
case 'number':
type = Number.isInteger(v) ? 'int' : 'real';
break;
case 'boolean':
type = 'bool';
break;
case 'object':
if (isDate(v)) {
type = 'date';
break;
}
default:
throw new Error(`Unsupported type: ${typeof v}`);
}

// convert to plural form for arrays
return isArray ? `${type}s` : type;
}

function isRecord(value: unknown): value is Record<string, unknown> {
return typeof value === 'object' && value !== null && !Array.isArray(value) && !isDate(value);
}
Comment on lines +78 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: could use _.isPlainObject

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as #129927 (comment): I wanted to avoid having another dependency in the package. If you think it's worth adding it, I don't mind adding it. :)


function isDate(value: unknown): value is Date {
return value instanceof Date || moment.isMoment(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit scary to know that we may have moment instances used for dates instead of plain Dates or timestamp. Do we really want to support that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove this. Technically, anything is possible. We are accepting unknown. And nothing is really stopping folks from setting a field to a moment instance. But let's remove it for now 👍

}
Loading