Rewrite JS Transformations to use Maps instead of Proxy Objects #1337

AndreKurait · 2025-03-10T18:55:54Z

Description

Improves performance and usability of Javascript Transforms by rewriting JS Transformations to use Maps instead of Proxy Objects.

This alone results in reduced performance hit with the Type Mapping Sanitization Transformer for RFS from 43% -> 18%.

Furthermore implemented batching with RFS transformations allowing RFS to send up to 100 documents at a time to downstream transformations which further reduced the performance hit for the Type Mapping Sanitization Transformer from 18% -> ~9%. To support this with existing non-batch-supporting transforms, added FlatteningJsonArrayTransformer which can wrap an existing transform to enable it to support the RFS batches.

In commit demonstrated unit testing javascript code with the type conversion from java -> javascript.

Issues Resolved

MIGRATIONS-2392

Testing

Added unit tests, ran in the cloud to verify overall performance improvements.

Check List

New functionality includes testing
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2025-03-10T19:49:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.19%. Comparing base (56bf099) to head (fd5e72b).
Report is 58 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1337      +/-   ##
============================================
+ Coverage     79.46%   80.19%   +0.72%     
+ Complexity     3263     2979     -284     
============================================
  Files           468      440      -28     
  Lines         16934    16184     -750     
  Branches       1115     1078      -37     
============================================
- Hits          13457    12978     -479     
+ Misses         2805     2570     -235     
+ Partials        672      636      -36

Flag	Coverage Δ
unittests	`80.19% <ø> (+0.72%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Andre Kurait <akurait@amazon.com>

…k transforms with existing transformations Signed-off-by: Andre Kurait <akurait@amazon.com>

Signed-off-by: Andre Kurait <akurait@amazon.com>

… converter Signed-off-by: Andre Kurait <akurait@amazon.com>

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait · 2025-03-12T16:12:55Z

...rmers/jsonTypeMappingsSanitizationTransformer/src/main/resources/js/typeMappingsSanitizer.js

@@ -20,6 +20,19 @@ function isEnabled(features, path) {
    return false;
 }

+function retargetCommandParameters(parameters, targetIndex) {


Moved in order to show up in commit history to demonstrate unit testing capabilities

Signed-off-by: Andre Kurait <akurait@amazon.com>

gregschohn · 2025-03-14T17:00:37Z

RFS/src/main/java/org/opensearch/migrations/bulkload/common/DocumentReindexer.java

@@ -34,7 +34,8 @@ public Flux<WorkItemCursor> reindex(String indexName, Flux<RfsLuceneDocument> do
        var scheduler = Schedulers.newParallel("DocumentBulkAggregator");
        var rfsDocs = documentStream
            .publishOn(scheduler, 1)
-            .concatMapIterable(doc -> transformDocument(threadSafeTransformer, doc, indexName));
+            .buffer(Math.min(100, maxDocsPerBulkRequest)) // arbitrary


should this be as big as the batch that we send out over the wire?

Not exactly, particularly for transformations that dramatically increase or decrease the size of the docs. Also, at this point we don't know how many documents we'll include over the wire, as most the time we construct it by a size instead of number

RFS/src/main/java/org/opensearch/migrations/bulkload/common/RfsDocument.java

...Transformer/src/test/java/org/opensearch/migrations/transform/JavascriptTransformerTest.java

...erface/src/main/java/org/opensearch/migrations/transform/FlatteningJsonArrayTransformer.java

gregschohn · 2025-03-14T18:10:47Z

...rmers/jsonTypeMappingsSanitizationTransformer/src/main/resources/js/typeMappingsSanitizer.js

+    return new Map([
+        ["method", "GET"],
+        ["URI", "/"],
+        ["protocol", "HTTP/1.0"]
+    ]);


I understand that we can send in a more constrained or different value, but should we prevent a user from being able to construct what that was? I'd suspect that this degree of friction will cause irritation.

I'm not following this comment regarding this function

I was wondering why this changed. Sounds like this format or the prior one will work for customers.

...rmers/jsonTypeMappingsSanitizationTransformer/src/main/resources/js/typeMappingsSanitizer.js

Signed-off-by: Andre Kurait <akurait@amazon.com>

gregschohn · 2025-03-18T21:49:02Z

...Transformer/src/test/java/org/opensearch/migrations/transform/JavascriptTransformerTest.java

-                    "    }; " +
-                    "}" +
-                    "main";
+    private static final String INIT_SCRIPT_2 = "\n" +


nit: this could just be ""+ too. It might be nice to put newlines within the string (like before 'main'). I will look at these const strings when debugging.

gregschohn · 2025-03-19T23:59:52Z

...rmers/jsonTypeMappingsSanitizationTransformer/src/main/resources/js/typeMappingsSanitizer.js

+    return new Map([
+        ["method", "GET"],
+        ["URI", "/"],
+        ["protocol", "HTTP/1.0"]
+    ]);


I was wondering why this changed. Sounds like this format or the prior one will work for customers.

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait temporarily deployed to migrations-cicd March 10, 2025 18:56 — with GitHub Actions Inactive

AndreKurait force-pushed the TransformationPerformance branch from b24f9c1 to d55ff80 Compare March 10, 2025 19:10

AndreKurait temporarily deployed to migrations-cicd March 10, 2025 19:10 — with GitHub Actions Inactive

AndreKurait temporarily deployed to migrations-cicd March 10, 2025 19:20 — with GitHub Actions Inactive

AndreKurait had a problem deploying to migrations-cicd March 10, 2025 21:49 — with GitHub Actions Failure

AndreKurait had a problem deploying to migrations-cicd March 10, 2025 22:37 — with GitHub Actions Failure

AndreKurait temporarily deployed to migrations-cicd March 12, 2025 06:47 — with GitHub Actions Inactive

AndreKurait changed the title ~~Transformation performance~~ Rewrite JS Transformations to use Maps over Proxy Objects Mar 12, 2025

AndreKurait temporarily deployed to migrations-cicd March 12, 2025 14:45 — with GitHub Actions Inactive

AndreKurait added 6 commits March 12, 2025 10:15

Add bulk RFS transformation processing with JS Maps

b5ea5b3

Signed-off-by: Andre Kurait <akurait@amazon.com>

Update CustomRfsTransformationTest to use js transform

be11410

Signed-off-by: Andre Kurait <akurait@amazon.com>

Update for javascript map

2b18c1b

Signed-off-by: Andre Kurait <akurait@amazon.com>

Fix null behavior

2b983f4

Signed-off-by: Andre Kurait <akurait@amazon.com>

Update type mappings behavior to tests

971318b

Signed-off-by: Andre Kurait <akurait@amazon.com>

Add FlatteningJsonArrayTransformerProvider to add support for RFS bul…

10d6c2d

…k transforms with existing transformations Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait force-pushed the TransformationPerformance branch from ce8b943 to 10d6c2d Compare March 12, 2025 15:15

AndreKurait temporarily deployed to migrations-cicd March 12, 2025 15:15 — with GitHub Actions Inactive

AndreKurait temporarily deployed to migrations-cicd March 12, 2025 15:17 — with GitHub Actions Inactive

Add plumbing to make javascript unit tests

742b7be

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait force-pushed the TransformationPerformance branch from 42fc8d5 to 742b7be Compare March 12, 2025 15:40

AndreKurait had a problem deploying to migrations-cicd March 12, 2025 15:40 — with GitHub Actions Failure

AndreKurait had a problem deploying to migrations-cicd March 12, 2025 15:43 — with GitHub Actions Failure

Cleanup case that is impossible in HttpJsonRequestWithFaultingPayload…

322bce2

… converter Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait force-pushed the TransformationPerformance branch from bffc95a to 322bce2 Compare March 12, 2025 15:45

AndreKurait had a problem deploying to migrations-cicd March 12, 2025 15:45 — with GitHub Actions Failure

Add example unit test for javascript function retargetCommandParameters

5f006bc

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait had a problem deploying to migrations-cicd March 12, 2025 15:46 — with GitHub Actions Failure

Update makeNoopRequest to return a map

f2b0972

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait marked this pull request as ready for review March 12, 2025 16:12

AndreKurait requested review from chelma, gregschohn, lewijacn, mikaylathompson, peternied, sumobrian and jugal-chauhan as code owners March 12, 2025 16:12

AndreKurait commented Mar 12, 2025

View reviewed changes

AndreKurait mentioned this pull request Mar 12, 2025

Add Performance Limitations to Readme #1346

Merged

2 tasks

Add tests to clarify javascript proxyMap/Object behavior

f9fc09c

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait temporarily deployed to migrations-cicd March 12, 2025 22:17 — with GitHub Actions Inactive

AndreKurait temporarily deployed to migrations-cicd March 12, 2025 22:31 — with GitHub Actions Inactive

Add map/object js case to get key that does not exist

2c10475

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait force-pushed the TransformationPerformance branch from 81a45ab to 2c10475 Compare March 12, 2025 22:31

AndreKurait temporarily deployed to migrations-cicd March 12, 2025 22:32 — with GitHub Actions Inactive

AndreKurait changed the title ~~Rewrite JS Transformations to use Maps over Proxy Objects~~ Rewrite JS Transformations to use Maps instead of Proxy Objects Mar 14, 2025

AndreKurait force-pushed the TransformationPerformance branch from 2c10475 to 88433d7 Compare March 14, 2025 17:04

AndreKurait temporarily deployed to migrations-cicd March 14, 2025 17:04 — with GitHub Actions Inactive

gregschohn reviewed Mar 14, 2025

View reviewed changes

Add test for immutableMap

fac4405

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait force-pushed the TransformationPerformance branch from 88433d7 to fac4405 Compare March 18, 2025 17:23

AndreKurait temporarily deployed to migrations-cicd March 18, 2025 17:23 — with GitHub Actions Inactive

Address PR Comments

ca6e300

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait temporarily deployed to migrations-cicd March 18, 2025 17:38 — with GitHub Actions Inactive

gregschohn approved these changes Mar 20, 2025

View reviewed changes

Rename luceneDocNumber to progressCheckpointNum to align with usage

fd5e72b

Signed-off-by: Andre Kurait <akurait@amazon.com>

AndreKurait temporarily deployed to migrations-cicd March 20, 2025 12:40 — with GitHub Actions Inactive

gregschohn approved these changes Mar 20, 2025

View reviewed changes

AndreKurait merged commit f913813 into opensearch-project:main Mar 20, 2025
60 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite JS Transformations to use Maps instead of Proxy Objects #1337

Rewrite JS Transformations to use Maps instead of Proxy Objects #1337

AndreKurait commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

AndreKurait Mar 12, 2025

gregschohn Mar 14, 2025

AndreKurait Mar 14, 2025

gregschohn Mar 14, 2025

AndreKurait Mar 18, 2025

gregschohn Mar 19, 2025

gregschohn Mar 18, 2025

gregschohn Mar 19, 2025

Rewrite JS Transformations to use Maps instead of Proxy Objects #1337

Rewrite JS Transformations to use Maps instead of Proxy Objects #1337

Conversation

AndreKurait commented Mar 10, 2025 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented Mar 10, 2025 • edited Loading

Codecov Report

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

AndreKurait commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading