New fsApi / version 2 support #235

irv-fs · 2024-12-10T00:02:20Z

No description provided.

… checks for that vs destination # [solrdeploy] uncomment this line if this requires solr deploy.

# [solrdeploy] uncomment this line if this requires solr deploy.

van-fs · 2024-12-14T14:02:08Z

src/observer.ts

@@ -9,6 +9,10 @@ import { FunctionOperator } from './operators';
 import DataLayerTarget from './target';
 import MonitorFactory from './monitor-factory';
 import { errorType, Telemetry, telemetryType } from './utils/telemetry';
+import TrackEventOperator from './operators/fsApi/trackEvent';


nit: these can be exported from the index.ts in /operators and imported as a single statement.

van-fs · 2024-12-14T14:05:12Z

src/observer.ts

  readOnLoad?: boolean;
  url?: string;
  id?: string;
  description?: string;
  monitor?: boolean;
  waitUntil?: number | Function;
  maxRetry?: number;
+  version?: number;
+  fsApi?: string;


fsApi is one of FS_API_CONSTANTS, no? Should that be the type and not string.

van-fs · 2024-12-14T14:07:20Z

src/observer.ts

+      if (fsApi) {
+        switch (fsApi) {
+          case FS_API_CONSTANTS.SET_IDENTITY:
+            handler.push(new SetIdentityOperator({ name: 'setIdentity' }));


We could just reuse the FS_API_CONSTANTS member as the value of name.

van-fs · 2024-12-14T14:10:21Z

src/observer.ts

   */
-  private addOperators(handler: DataHandler, options: OperatorOptions[], destination: string | Function) {
+  private addOperators(handler: DataHandler, options: OperatorOptions[],
+    destination: string | Function | undefined = undefined, fsApi: string | undefined = undefined,


destination and fsApi can be declared optional with destination? and fsApi?. Is there a need to add a union type with undefined and assign it undefined? That would be automatic with the optional.

I started with this, but optional parameters have to go at the end, and version is optional but with a hardcoded default value. I'd have to move version in front of them, and then anyone using the api would have to put a version in if they wanted to specify destination or fsApi. The other option would be to give these default values of undefined as well. Thoughts?

van-fs · 2024-12-14T14:41:04Z

src/observer.ts

+      return;
+    }
+
+    if (destination && fsApi) {


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.

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.

src/operators/fsApi/fsApi.ts

van-fs · 2024-12-14T14:53:29Z

src/observer.ts

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


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.

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.

src/operators/fsApi/setIdentity.ts

van-fs · 2024-12-14T15:08:01Z

src/operators/fsApi/setPageProperties.ts

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


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.

ScottLNorvell

Hi @irv-fs ! Thanks for your patience on this. Everything looks good here. I'm going to go schedule some time to go over this with @ndonze-fs to make sure I understand what's going on and then we'll

ndonze-fs · 2024-12-23T18:09:54Z

src/observer.ts

      // useful if every rule needs the same operator run before the destination
-      if (beforeDestination) {
+      if (beforeDestination && (version === 1)) {


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.

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.

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.

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.

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.

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.

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?

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?

ndonze-fs · 2024-12-23T18:11:19Z

src/observer.ts

-  private addOperators(handler: DataHandler, options: OperatorOptions[], destination: string | Function) {
+  private addOperators(handler: DataHandler, options: OperatorOptions[],
+    destination: string | Function | undefined = undefined, fsApi: string | undefined = undefined,
+    version: number = 1) {


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.

Added jsDoc. That was an oversight.

# [solrdeploy] uncomment this line if this requires solr deploy.

ndonze-fs · 2025-01-08T17:49:01Z

src/observer.ts

      Telemetry.error(errorType.invalidRuleError);
      return;
    }

+    // sanity check destination and fsApi parameters
+    if (!destination && !fsApi) {


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' }.

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).

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. 😆

ndonze-fs · 2025-01-08T17:53:37Z

src/observer.ts

      // useful if every rule needs the same operator run before the destination
-      if (beforeDestination) {
+      if (beforeDestination && (version === 1)) {


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.

ScottLNorvell

This all looks good from my end. I think once @ndonze-fs signs off we can get this deployed.

Sorry for the delay on this.

# [solrdeploy] uncomment this line if this requires solr deploy./gd

ndonze-fs

LGTM

# [solrdeploy] uncomment this line if this requires solr deploy./gd

irv-fs added 4 commits December 5, 2024 16:50

First surgery on getting version support written. Adding in fsApi and…

66edd6a

… checks for that vs destination # [solrdeploy] uncomment this line if this requires solr deploy.

First surgery on getting version support written. Adding in fsApi and…

490bbea

… checks for that vs destination # [solrdeploy] uncomment this line if this requires solr deploy.

Start of unit testing fsApi

27be27c

# [solrdeploy] uncomment this line if this requires solr deploy.

Checkin of working code and tests

978a1e8

# [solrdeploy] uncomment this line if this requires solr deploy.

irv-fs assigned irv-fs, ndonze-fs and van-fs and unassigned irv-fs Dec 10, 2024

irv-fs added 2 commits December 9, 2024 19:08

Fixing accidental change of doc

687e86b

# [solrdeploy] uncomment this line if this requires solr deploy.

Additional test for ignoring before destination on version 2

2acb818

# [solrdeploy] uncomment this line if this requires solr deploy.

van-fs reviewed Dec 14, 2024

View reviewed changes

src/operators/fsApi/fsApi.ts Show resolved Hide resolved

van-fs reviewed Dec 14, 2024

View reviewed changes

src/operators/fsApi/setIdentity.ts Show resolved Hide resolved

van-fs reviewed Dec 14, 2024

View reviewed changes

ScottLNorvell reviewed Dec 16, 2024

View reviewed changes

ndonze-fs reviewed Jan 2, 2025

View reviewed changes

irv-fs added 2 commits January 2, 2025 13:53

Update for code review changes

cee4a1a

# [solrdeploy] uncomment this line if this requires solr deploy.

Update for code review changes

ba05877

# [solrdeploy] uncomment this line if this requires solr deploy.

ndonze-fs reviewed Jan 8, 2025

View reviewed changes

ScottLNorvell approved these changes Jan 8, 2025

View reviewed changes

Adding in dlo as parameter to version 2 destination rules

591634f

# [solrdeploy] uncomment this line if this requires solr deploy./gd

ndonze-fs approved these changes Jan 16, 2025

View reviewed changes

Update documentation

f526702

# [solrdeploy] uncomment this line if this requires solr deploy./gd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New fsApi / version 2 support #235

New fsApi / version 2 support #235

irv-fs commented Dec 10, 2024

van-fs Dec 14, 2024

irv-fs Jan 2, 2025

van-fs Dec 14, 2024

irv-fs Jan 2, 2025

van-fs Dec 14, 2024 •

edited

Loading

irv-fs Jan 2, 2025

van-fs Dec 14, 2024 •

edited

Loading

irv-fs Jan 2, 2025

van-fs Dec 14, 2024

irv-fs Jan 2, 2025

van-fs Dec 14, 2024

irv-fs Jan 2, 2025

van-fs Dec 14, 2024

irv-fs Jan 2, 2025

ScottLNorvell left a comment

ndonze-fs Dec 23, 2024

irv-fs Jan 2, 2025

irv-fs Jan 2, 2025

ndonze-fs Jan 8, 2025

irv-fs Jan 8, 2025

ndonze-fs Jan 8, 2025

ndonze-fs Jan 8, 2025

irv-fs Jan 8, 2025

ndonze-fs Dec 23, 2024

irv-fs Jan 2, 2025

ndonze-fs Jan 8, 2025

irv-fs Jan 8, 2025

ndonze-fs Jan 8, 2025

ndonze-fs Jan 8, 2025

ScottLNorvell left a comment

ndonze-fs left a comment

New fsApi / version 2 support #235

Are you sure you want to change the base?

New fsApi / version 2 support #235

Conversation

irv-fs commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

van-fs Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

van-fs Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottLNorvell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottLNorvell left a comment

Choose a reason for hiding this comment

ndonze-fs left a comment

Choose a reason for hiding this comment

van-fs Dec 14, 2024 •

edited

Loading

van-fs Dec 14, 2024 •

edited

Loading