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

New fsApi / version 2 support #235

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
99 changes: 83 additions & 16 deletions src/observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import DataHandler from './handler';
import {
Logger, LogAppender, LogMessageType, LogMessage, LogLevel,
} from './utils/logger';
import { FunctionOperator } from './operators';
import {
FunctionOperator, SetIdentityOperator, SetPagePropertiesOperator, SetUserPropertiesOperator, TrackEventOperator,
} from './operators';
import DataLayerTarget from './target';
import MonitorFactory from './monitor-factory';
import { errorType, Telemetry, telemetryType } from './utils/telemetry';
Expand Down Expand Up @@ -60,14 +62,23 @@ export interface DataLayerRule {
debug?: boolean;
source: string;
operators?: OperatorOptions[];
destination: string | Function;
destination?: string | Function;
readOnLoad?: boolean;
url?: string;
id?: string;
description?: string;
monitor?: boolean;
waitUntil?: number | Function;
maxRetry?: number;
version?: number;
fsApi?: FS_API_CONSTANTS;
}

export enum FS_API_CONSTANTS {
SET_IDENTITY = 'setIdentity',
TRACK_EVENT = 'trackEvent',
SET_USER_PROPERTIES = 'setUserProperties',
SET_PAGE_PROPERTIES = 'setPageProperties'
}

/**
Expand Down Expand Up @@ -190,8 +201,13 @@ export class DataLayerObserver {
* adding the operator, the DataHandler will be removed to prevent unexpected data processing.
* @param handler to add operators to
* @param options for operators used to configure each Operator
* @param destination The javascript function to execute (must be one of destination or fsApi)
* @param fsApi The special FullStory constant to be executed (must be one of destination or fsApi)
* @param version The version of DLO you are using. As of version 2, beforeDestination will not be used.
*/
private addOperators(handler: DataHandler, options: OperatorOptions[], destination: string | Function) {
private addOperators(handler: DataHandler, options: OperatorOptions[],
destination: string | Function | undefined = undefined, fsApi: FS_API_CONSTANTS | undefined = undefined,
version: number = 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

version feels generic here without a fuller name/jsdoc. Wonder if we should make this clear that we're referring to FS client API version -- and maybe further constrain to known versions with a union instead of using a 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.

Added jsDoc. That was an oversight.

const { beforeDestination, previewDestination = 'console.log', previewMode } = this.config;

try {
Expand All @@ -200,16 +216,36 @@ export class DataLayerObserver {
handler.push(this.getOperator(optionSet));
});

// optionally perform a final transformation
// optionally perform a final transformation if version is 1
// useful if every rule needs the same operator run before the destination
if (beforeDestination) {
if (beforeDestination && (version === 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this prevent the application of any beforeDestination operators? If so, is this safe/intended? I expect we're trying to avoid having the suffix operator added. However, we also convert by default and insert the dlo capture source (not sure how capture sources work in v2 APIs).

[{ name: 'convert', enumerate: true, preserveArray: true, index: -1 },{ name: 'suffix' },{ name: 'insert', value: 'dlo', position: -1 }]

I wonder if instead we should be thinking about noop'ing the suffix operator given v2 client APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the way this is coded, if you use V2 of DLC you are in charge of putting all of your operators that are needed, which would include the convert operator. I do automatically insert dlo as an additional parameter in the fsApi base operator, so that piece is taken care of. You can use the new fsApi stuff with version 1 however if desired.

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 was very nervous to just make the suffix operator a no-op. I know we have done a lot of work to get around suffixes, allowing people to use specific names. I instead decided to introduce a rule version, which I think gives us more flexibility. Anybody who wants this new functionality needs to explicitly set the rule version, which they knowingly opt into. We can also use this for future backwards incompatible changes at the rule level. I think it gives us another lever that was missing in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

A side effect of nixing beforeDestinationOperators is that moving to FS API V2 is coupled with a capture behavior change. Previously, we added convert for you. Now, given the same set of rules upgraded to V2, we'll no longer convert. if you had a string amount e.g. "10.75", we would've previously been converting that to a real. Now, we'll no longer do that. So, we both stop adding suffixes (good relative to V2) and we change the capture behavior. If a customer would be negatively impacted by the change in capture behavior (e.g. now having mixed real and str values), they can't be easily be migrated to FS API V2.

This coupling doesn't feel great for me as it limits the on-ramp to V2. I wonder if it's worth soliciting a product perspective on this decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a customer moves to V2, it won't be as easy as flipping a switch or setting a simple attribute as each rule will have to be visited as some inserts, etc will need to be removed. The structure of V2 is just different, so it won't be a simple path regardless. Forcing them to also consider convert seems small compared to what is already needed, and gets us away from the hardcoded beforeDestination stuff while keeping backwards compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide examples of other concerns we'll have moving customers from V1 to V2? You mentioned the structure of V2 being different which suggests that -- even if we were to position the V2 API as a destination instead of an operator -- we wouldn't be able to take an existing operator chain and have it call V2 APIs without capture behavior changes.

I'm pulling on this thread because in the past we've seen capture behavior changes (e.g. when we've changed beforeDestinationOperators) as breaking changes which motivated us to:

  • Bump the DLO major version
  • Feature switch such that we could migrate customers between major versions over time

In this case, it's a little different because we're introducing a rule version within the overall DLO library version.

Between data sinks becoming operators and opting into breaking changes via rule specs, we're shifting responsibilities around vs. where they've lived in the past. This may be okay, though I think we should have a good awareness of these shifts as they may complicate what changes we can make and where over time e.g. not being able to count on beforeDestinationOperators to make changes with client integration scripts since we can no longer count on those to be effective for all customers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking back on an earlier comment, is this true?

You can use the new fsApi stuff with version 1 however if desired.

With FS API calls currently being positioned as operators instead of destinations, wouldn't the operator to call FS API V2 execute before the beforeDestinationOperators which are appended to the end of the operators array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opening up a rule version would allow those writing rules to gradually migrate as they see fit, even rule by rule, versus an all or nothing change, and doesn't require any major hassles on our end (as far as migrating folks). Given that most of the rule writers are inside Fullstory, this seems to be a benefit?

const beforeOptions = Array.isArray(beforeDestination) ? beforeDestination : [beforeDestination];
beforeOptions.forEach((operator) => handler.push(this.getOperator(operator)));
}

// end with destination
const func = previewMode ? previewDestination : destination;
handler.push(new FunctionOperator({ name: 'function', func }));
if (fsApi) {
Copy link
Member

Choose a reason for hiding this comment

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

Given these are operators, my skew is that we re-use the OperatorFactory and this.getOperator to instantiate operators. I may have to get into the code to see if that creates unwanted complexity.

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 wanted it explicit in the code, but could move those into registered operators with an OperatorFactory if desired. I tend to favor explicit code in special situations like this, but not a hill for me to die on.

switch (fsApi) {
case FS_API_CONSTANTS.SET_IDENTITY:
handler.push(new SetIdentityOperator({ name: FS_API_CONSTANTS.SET_IDENTITY }));
break;
case FS_API_CONSTANTS.SET_PAGE_PROPERTIES:
handler.push(new SetPagePropertiesOperator({ name: FS_API_CONSTANTS.SET_PAGE_PROPERTIES }));
break;
case FS_API_CONSTANTS.SET_USER_PROPERTIES:
handler.push(new SetUserPropertiesOperator({ name: FS_API_CONSTANTS.SET_USER_PROPERTIES }));
break;
case FS_API_CONSTANTS.TRACK_EVENT:
handler.push(new TrackEventOperator({ name: FS_API_CONSTANTS.TRACK_EVENT }));
break;
default:
Logger.getInstance().error(`Unexpected coding error: Unknown fsApi value ${fsApi}`);
}
} else if (destination) {
const func = previewMode ? previewDestination : destination;
handler.push(new FunctionOperator({ name: 'function', func }));
} else {
Logger.getInstance().error('Unexpected coding error: Missing fsApi or destination');
}
} catch (err) {
this.removeHandler(handler);
Logger.getInstance().error(LogMessageType.OperatorError, { operator: JSON.stringify(options) });
Expand Down Expand Up @@ -263,22 +299,26 @@ export class DataLayerObserver {
* @param source from the rule monitoring the data layer
* @param target from the data layer
* @param options list of OperatorOptions to transform data before a destination
* @param destination function using selector syntax or native function
* @param read when true reads data layer target and emit the initial value
* @param monitor when true property changes or function calls re-run the operators
* @param debug when true the rule prints debug for each Operator transformation
* @param debounce number of milliseconds to debounce property assignments before handling the event
* @param version version of this rule, defaults to 1
* @param destination function using selector syntax or native function
* @param fsApi special Fullstory API Constant
* @throws error if an error occurs during handler creation
*/
registerTarget(
source: string,
target: DataLayerTarget,
options: OperatorOptions[],
destination: string | Function,
destination: string | Function | undefined = undefined,
fsApi: FS_API_CONSTANTS | undefined = undefined,
read = false,
monitor = true,
debug = false,
debounce = DataHandler.DefaultDebounceTime,
version:number = 1,
): DataHandler {
let workingTarget = target;
const targetValue = workingTarget.value;
Expand All @@ -289,8 +329,8 @@ export class DataLayerObserver {
*/
if (monitor && Array.isArray(targetValue)) {
if (targetValue.push && targetValue.unshift) {
this.registerTarget(source, DataLayerTarget.find(`${target.path}.unshift`), options, destination, false, true,
debug, debounce);
this.registerTarget(source, DataLayerTarget.find(`${target.path}.unshift`), options, destination, fsApi,
false, true, debug, debounce, version);
workingTarget = DataLayerTarget.find(`${target.path}.push`);
} else {
Logger.getInstance().warn(LogMessageType.MonitorCreateError, {
Expand All @@ -303,7 +343,7 @@ export class DataLayerObserver {
}

const handler = this.addHandler(source, workingTarget, !!debug, debounce);
this.addOperators(handler, options, destination);
this.addOperators(handler, options, destination, fsApi, version);

if (read) {
// For read-on-load for targeted arrays we do a sort of manual fan-out of the items
Expand Down Expand Up @@ -408,6 +448,8 @@ export class DataLayerObserver {
source,
operators = [],
destination,
fsApi,
version,
readOnLoad: ruleReadOnLoad,
url,
monitor = true,
Expand All @@ -417,13 +459,37 @@ export class DataLayerObserver {
// rule properties override global ones
const readOnLoad = ruleReadOnLoad === undefined ? globalReadOnLoad : ruleReadOnLoad;

if (!source || !destination) {
if (!source) {
Logger.getInstance().error(LogMessageType.RuleInvalid,
{ rule: id, source, reason: `Missing ${source ? 'destination' : 'source'}` });
{ rule: id, source, reason: 'Missing source' });
Telemetry.error(errorType.invalidRuleError);
return;
}

// sanity check destination and fsApi parameters
if (!destination && !fsApi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking a bit more on this. If we think about DLO as ETL (extract from source, transform through operators, load through destination), as implemented this positions the FS API V2 destination as an operator rather than a destination. How do we feel about this overall positioning? Operators can now be transformations, or they could actually be sending data -- whereas previously there was a separation between operators (transformations) and destinations (data sinks). As a side effect, this kind of forces us away from beforeDestinationOperators because with V2 there is no destination.

As a potential alternative to consider, we could position destination as a string (as is) or as an FS API V2 destination which would be a more complex destination object. As a crude example, destination: { target: 'fs', method: 'setIdentity' }.

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 my design doc I had originally positioned the V2 as just a special string inside a destination, but it seemed more folks wanted the separate attribute (which I also prefer as it is clearer the intention).

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, we wouldn't have a special string; rather, we would have a more complex destination object that wouldn't look terribly different than operator definitions. That said, this is not a hill I will die on if other folks think it's sensible to start merging the ideas of operators/transformations and data sinks. 😆

Logger.getInstance().error(LogMessageType.OperatorError, {
reason: LogMessage.MissingDestination,
});
Telemetry.error(errorType.operatorError);
return;
}

if (destination && fsApi) {
Copy link
Member

Choose a reason for hiding this comment

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

While conceptually incorrect (competing destinations), I wonder if we should prefer fsApi if it is also used with destination. In this scenario, we still still send data versus nothing. We could emit an error to indicate the problem, and the user would be able to see the event in the replay along with the indication there are competing destinations.

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 tend to think if someone specified both it was probably done in error, and I'd rather bomb out than have them hunt down why it isn't working. This happens with DLO unfortunately and it can be tricky to figure out why things are broken. But not a hill for me to die on either, so could change it.

Logger.getInstance().error(LogMessageType.OperatorError, {
reason: LogMessage.DuplicateDestination,
});
Telemetry.error(errorType.operatorError);
return;
}

if (fsApi && !Object.values(FS_API_CONSTANTS).includes(fsApi as FS_API_CONSTANTS)) {
const reason = Logger.format(LogMessage.UnsupportedFsApi, fsApi);
Logger.getInstance().error(LogMessageType.OperatorError, { reason });
Telemetry.error(errorType.operatorError);
return;
}

// check the rule is valid for the url
if (!this.isUrlValid(url)) {
return;
Expand All @@ -432,7 +498,8 @@ export class DataLayerObserver {
try {
const register = () => {
const target = DataLayerTarget.find(source);
this.registerTarget(source, target, operators, destination, readOnLoad, monitor, debug, debounce);
this.registerTarget(source, target, operators, destination, fsApi, readOnLoad, monitor, debug,
debounce, version);
};
const timeout = () => Logger.getInstance().warn(LogMessageType.RuleRegistrationError, {
rule: id, source, reason: 'Max Retries Attempted',
Expand Down
42 changes: 42 additions & 0 deletions src/operators/fsApi/fsApi.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { Operator, OperatorOptions, OperatorValidator } from '../../operator';
import { getGlobal } from '../../utils/object';

export interface FSApiOperatorOptions extends OperatorOptions {

}

export default abstract class FSApiOperator implements Operator {
static specification = {};

constructor(public options:FSApiOperatorOptions) {
// sets this.options
}

handleData(data: any[]): any[] | null {
const thisArg: object = getGlobal();
// @ts-ignore
const fsFunction:any = thisArg[thisArg._fs_namespace]; // eslint-disable-line
van-fs marked this conversation as resolved.
Show resolved Hide resolved
if (typeof fsFunction !== 'function') {
throw new Error('_fs_namespace is not a function');
}
// subclasses will determine how to prepare the data
const realData = this.prepareData(data);
if (realData === null) {
return null;
}
// make sure to push dlo as last parameter
realData.push('dlo');
const returnValue = fsFunction.apply(thisArg, realData);
if (returnValue === undefined || returnValue === null) {
return null;
}
return [returnValue];
}

validate() {
const validator = new OperatorValidator(this.options);
validator.validate(FSApiOperator.specification);
}

abstract prepareData(inputData:any[]): any[] | null;
}
21 changes: 21 additions & 0 deletions src/operators/fsApi/setIdentity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import FSApiOperator from './fsApi';

// eslint-disable-next-line import/prefer-default-export
export class SetIdentityOperator extends FSApiOperator {
// eslint-disable-next-line class-methods-use-this
prepareData(inputData:any[]): any[] | null {
if (inputData === null || inputData.length < 1) {
throw new Error('Input data is empty');
}
const realData:any[] = [
van-fs marked this conversation as resolved.
Show resolved Hide resolved
'setIdentity',
{ uid: inputData[0] },
];
// setIdentity calls can have optional properties
if (inputData.length > 1) {
// eslint-disable-next-line prefer-destructuring
realData[1].properties = inputData[1];
}
return realData;
}
}
18 changes: 18 additions & 0 deletions src/operators/fsApi/setPageProperties.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import FSApiOperator from './fsApi';

// eslint-disable-next-line import/prefer-default-export
export class SetPagePropertiesOperator extends FSApiOperator {
// eslint-disable-next-line class-methods-use-this
prepareData(inputData:any[]): any[] | null {
if (inputData === null || inputData.length < 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we throw an Error? The operator expects inputData, right? Is there a scenario where not receiving data but expecting it to be sent to setProperties is OK? Returning null is typically used to halt the operator chain under expected conditions - like when a query doesn't match and the chain is expected to stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

throw new Error('Input data is empty');
}
return [
'setProperties',
{
type: 'page',
properties: inputData[0],
},
];
}
}
18 changes: 18 additions & 0 deletions src/operators/fsApi/setUserProperties.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import FSApiOperator from './fsApi';

// eslint-disable-next-line import/prefer-default-export
export class SetUserPropertiesOperator extends FSApiOperator {
// eslint-disable-next-line class-methods-use-this
prepareData(inputData:any[]): any[] | null {
if (inputData === null || inputData.length < 1) {
throw new Error('Input data is empty');
}
return [
'setProperties',
{
type: 'user',
properties: inputData[0],
},
];
}
}
20 changes: 20 additions & 0 deletions src/operators/fsApi/trackEvent.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import FSApiOperator from './fsApi';

// eslint-disable-next-line import/prefer-default-export
export class TrackEventOperator extends FSApiOperator {
// eslint-disable-next-line class-methods-use-this
prepareData(inputData:any[]): any[] | null {
if (inputData === null || inputData.length < 1) {
throw new Error('Input data is empty');
} else if (inputData.length < 2) {
throw new Error('Input data expected to have two parameters');
}
return [
'trackEvent',
{
name: inputData[0],
properties: inputData[1],
},
];
}
}
4 changes: 4 additions & 0 deletions src/operators/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ export * from './suffix';
export * from './rename';
export * from './query';
export * from './parse';
export * from './fsApi/setIdentity';
export * from './fsApi/setPageProperties';
export * from './fsApi/setUserProperties';
export * from './fsApi/trackEvent';
3 changes: 3 additions & 0 deletions src/utils/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ export enum LogMessageType {
export enum LogMessage {
DataLayerMissing = 'Data layer not found',
DuplicateValue = 'Value $0 already used',
DuplicateDestination = 'Only one of destination or fsApi can be defined',
MissingDestination = 'destination or fsApi must be defined',
ShimFail = 'Shim not allowed because object is $0',
SelectorInvalidIndex = 'Selector index $0 is not a number in $1',
SelectorIncorrectTokenCount = 'Selector has incorrect number ($0) of tokens in $1',
Expand All @@ -45,6 +47,7 @@ export enum LogMessage {
TargetPropertyMissing = 'Target property is missing',
TargetPathMissing = 'Target path is missing',
UnknownValue = 'Unknown value $0',
UnsupportedFsApi = 'Unsupported fsApi $0',
UnsupportedType = 'Unsupported type $0',
}

Expand Down
13 changes: 13 additions & 0 deletions test/mocks/fullstoryV2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
let callQueues: any[] = [];

export function fullstoryMock(...args:any[]) {
callQueues.push(args);
}

export function clearCallQueues() {
callQueues = [];
}

export function getCallQueues() : any {
return callQueues;
}
Loading