-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Changes from 9 commits
66edd6a
490bbea
27be27c
978a1e8
687e86b
2acb818
cee4a1a
ba05877
591634f
f526702
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,14 @@ import DataHandler from './handler'; | |
import { | ||
Logger, LogAppender, LogMessageType, LogMessage, LogLevel, | ||
} from './utils/logger'; | ||
import { FunctionOperator } from './operators'; | ||
import { | ||
FunctionOperator, | ||
InsertOperator, | ||
SetIdentityOperator, | ||
SetPagePropertiesOperator, | ||
SetUserPropertiesOperator, | ||
TrackEventOperator, | ||
} from './operators'; | ||
import DataLayerTarget from './target'; | ||
import MonitorFactory from './monitor-factory'; | ||
import { errorType, Telemetry, telemetryType } from './utils/telemetry'; | ||
|
@@ -60,14 +67,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' | ||
} | ||
|
||
/** | ||
|
@@ -190,8 +206,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) { | ||
const { beforeDestination, previewDestination = 'console.log', previewMode } = this.config; | ||
|
||
try { | ||
|
@@ -200,16 +221,42 @@ 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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this prevent the application of any
I wonder if instead we should be thinking about noop'ing the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A side effect of nixing 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking back on an earlier comment, is this true?
With FS API calls currently being positioned as operators instead of destinations, wouldn't the operator to call FS API V2 execute before the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given these are operators, my skew is that we re-use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
// if the version is greater than 1 it should ignore beforeDestination but still add dlo output | ||
if (version > 1) { | ||
handler.push(new InsertOperator({ | ||
name: 'insert', position: -1, value: 'dlo', | ||
})); | ||
} | ||
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) }); | ||
|
@@ -263,22 +310,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; | ||
|
@@ -289,8 +340,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, { | ||
|
@@ -303,7 +354,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 | ||
|
@@ -408,6 +459,8 @@ export class DataLayerObserver { | |
source, | ||
operators = [], | ||
destination, | ||
fsApi, | ||
version, | ||
readOnLoad: ruleReadOnLoad, | ||
url, | ||
monitor = true, | ||
|
@@ -417,13 +470,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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -432,7 +509,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', | ||
|
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; | ||
} |
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; | ||
} | ||
} |
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we throw an There was a problem hiding this comment. Choose a reason for hiding this commentThe 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], | ||
}, | ||
]; | ||
} | ||
} |
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], | ||
}, | ||
]; | ||
} | ||
} |
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], | ||
}, | ||
]; | ||
} | ||
} |
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; | ||
} |
There was a problem hiding this comment.
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 anumber
.There was a problem hiding this comment.
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.