-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add JS transformation example - Amplitude #206
base: master
Are you sure you want to change the base?
Conversation
e3e2f0a
to
9486b32
Compare
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.
(Note that my comments are not from a technical point of view, but from educational, i.e. how can we ensure this example is clear for the user.)
return props; | ||
}; | ||
|
||
const replaceAll = (str, substr, newSubstr) => { |
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.
What is the advantage of this over str.replace(/pattern/g, ...)
? Is it that it handles the situations when the replacement contains the pattern?
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.
Thank you for reviewing @stanch !
Most of the code for this example comes from the Amplitude GTM-SS Tag. In the GTM-SS environment we are running the so called "sandboxed" or "standard" JavaScript library, along with some provided API's. This is why in many cases we have to work around some (not always perfectly documented) limitations.
In this particular instance: replaceAll
aims to replicate exactly the String.prototype.replace()
where the pattern
is a RegEx, because we don't have RegEx
in this GTM standard library to create the pattern from a variable.
I am now not sure whether this was the right call, but so far i have tried to keep as much of the gtm-ss template code as possible, which means we don't always make perfect use of full ECMAScript 5.1 that goja
supports. You are right that from the "example" point of view, this may be more confusing than it needs.
* @param obj {Object} - the object to look into | ||
* @returns - the corresponding value or undefined | ||
*/ | ||
const getFromPath = (path, obj) => { |
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.
I am curious, do we allow to import libraries in Stream Replicator scripts? A lot of these helper functions look like something that already exists elsewhere (in this case, https://lodash.com/docs/4.17.15#get). IMHO it would be easier for people to follow if we focused on the principal logic here.
The larger context behind this comment is that while I think having a complete working example is awesome, I also predict it will scare people that this example has a whopping 700 LOC :)
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.
Indeed, i am not sure if this serves best as an example..
The way to load libraries would be to compile them along with the script, in order for that JavaScript code to be evaluated along. I haven't tried yet but i do expect this to work, as also described here. So yes in theory, someone could load lodash
if they wanted to by including it in their script.
An alternative would be to provide something from Go
side, but i haven't explored options (e.g. adding some require
functionality) and i don't know if this is desired at this point. cc @colmsnowplow
* Performs string concatenation, so assumes the types of its arguments are | ||
* strings, numbers or booleans. | ||
*/ | ||
const mkDims = (width, height) => { |
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.
width && height && `${width}x${height}`
?
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.
(Following the comment above, just wondering if we can shorten some of these things)
return new Date(isoTime).getTime(); | ||
} | ||
|
||
const cleanObject = (obj) => { |
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.
https://lodash.com/docs/4.17.15#omitBy
_omitBy(obj, _.isNil)
etc... But if we don’t support libraries then I would seriously consider implementing that support, because getting things done without Lodash is really annoying :(
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.
We do support libraries. It is just that they need to be explicitly included along!
Just sharing some thoughts:
On one hand i understand that there exist very useful JavaScript libraries that are commonly used. On the other hand, if it is only for a couple of functions, why not go "vanilla"? I mean that if it is enough for people to use GTM, which is already more restrictive, it may also be enough for transformation scripts.
Another point is that if we start by supporting a list of commonly useful libraries (e.g. by providing additional API's), i am afraid that it may be hard to keep that list limited.
What do you think?
let insertId = evData.event_id; | ||
let platform = evData.platform; | ||
|
||
let amplitudeEvent = { |
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.
Why let and not const?
Just popping this here for posterity since I've now spoken to both people. This PR was originaly created as Ada wanted to spike out whether we could port the GTM tag to Stream replicator, and I wanted to see what it looked like if it worked (which it does, so awesome!). It should really be thought of as more of a hackathonish spike than anything, so we don't expect the comments to be addressed for the moment, and we don't need to prioritise getting it into a mergeable state at least for a bit. It is, however, an awesome metre stick for improvements to scripting transformations in the future. I think Nick has already identified some things - like making it clear how to use libraries. I think figuring out how to simplify this kind of example is a great way to assess where the best kind of improvements can be found, and also this kind of example is a great thing to use in a higher-load performance test. I'm going to convert it to draft for now, and we can return to and learn from it. Thanks again Ada! |
This is super-cool! Nice work @adatzer and @colmsnowplow. I can see a future where we de-emphasize the "built for GTM SS" nature of the tags and instead make clear that our library of tags supports two runtimes: 1. GTM SS and 2. stream-replicator. |
Definitely a future I'd be interested in @alexanderdean . IMO a library of Snowplow-built and externally-contributed tags/scripts would be super cool - and from there, the most popular ones could maybe be adopted into faster native go transformations. |
Yes that would be cool - we could embed some transformers in Go, or have a binary bridge like AWS Lambda so you could write them in Rust or Zig or whatever... |
This is a first JavaScript transformation example.
The example is a full port of the Snowplow GTM-SS logic for Amplitude:
snowplow_mode=true
Pros:
time.Time
to JavaScript and how to handle it.Cons:
Even though not a ready PR, opening it to get some feedback whether this is something we want.