feat: Add json_mode to JS transformation

[Mikhail (oo-sleeper)]

ref PDP-1983

json_mode is a new setting for JS transformation, which should make it more transparent into what data is expected as an input to the transformations

If the data is in JSON format, then it would be converted into an object to use in JS script
If the data is not in JSON format, transformation would fail

[Phil Catterall (phil-snowplow)]

One minor quip about naming: if this supports any JSON input and snowplow TSV, then snowplow_mode doesn't really describe it accurately: it's actually snowplow_tsv_and_any_json mode. Seems confusing from a user perspective.

Also, I see we try to parse as tsv and if that fails, parse as json. Is that going to add any unnecessary overhead in cases where we know the input is always going to be json? Or are we not concerned about that?

[colmsnowplow]

It's a bit rock and hard place Phil Catterall (@phil-snowplow) - I'm happy to hear suggestions for a better name!

If this supports any JSON input and snowplow TSV, then snowplow_mode doesn't really describe it accurately: it's actually snowplow_tsv_and_any_json mode. Seems confusing from a user perspective

My take is this:

• We have an existing snowplow_mode which only works with snowplow data
• We're extending it to work with another form of Snowplow data
• By happenstance it'll also work with json data

Nothing actually changes for someone whose data was another JSON - except that there's now a workaround. I'd rather not have that be advertised support for any JSON, simply because I don't think there's any real ask from users to do so. So I'm ok with it being named this even though it happens to also do something else.

I guess the goal isn't to describe the precise functionality as accurately as possible, but to describe the use case we're supporting with it.

Which leads to the answer to:

Is that going to add any unnecessary overhead in cases where we know the input is always going to be json? Or are we not concerned about that?

Yeah it does, if we were doing generic JSON support I'd have a concern and design it differently, but because we're supporting a specific internal use case here, I don't.

The order of operations should be TSV first because that's the prod scenario where latency matters. The other use case is in a testing flow so it's fine to have the extra bit of work.

Next breaking release I think we should clean this up. Then, we can do it as follows:

• Assume the data is JSON and try to provide it as such
• Have this feature for TSV/JSON
• We can also configure it to provide the data as a string instead, no matter what the data is

[Mikhail (oo-sleeper)]

Next breaking release I think we should clean this up. Then, we can do it as follows:

• Assume the data is JSON and try to provide it as such
• Have this feature for TSV/JSON
• We can also configure it to provide the data as a string instead, no matter what the data is

on this one I'd probably suggest something a bit more breaking, but making more sense long-term

at the source level, explicitly specify what the data format is (start with JSON/TSV)
and thus reduce any need to hassle with translations/figuring out what's going on in transformations

from there on, we would be able to add support for anything else (be it CSV or just plain strings) with clearer logic how it gets transformed

[Phil Catterall (phil-snowplow)]

Yeah it does, if we were doing generic JSON support I'd have a concern and design it differently, but because we're supporting a specific internal use case here, I don't.

colmsnowplow that's a fair point – if we assume JSON mode won't be used at scale, just for testing, and if we're purposefully avoiding breaking changes, I agree the current approach is the best one.

Then, we can backlog Mikhail (@oo-sleeper)'s suggestion for the next time we want to do a breaking release.

[colmsnowplow]

Looks good! Please Rename PR and clean up/give it an accurate commit name once you're done.