[Synthetics] Template string syntax breaks inline monitors when running against the service #176094

justinkambic · 2024-02-01T19:28:47Z

Summary

~~Code is in WIP state.~~

Makes it so inline monitor scripts can make use of JavaScript template strings.

This PR will migrate data from the inline fields to use the project fields instead. This approach combined with b64 encoding will allow us to avoid the instance where we try to put JS template strings in dynamic YML, which breaks the YML syntax.

Testing instructions

Pre-requisites

Create a script to break the service. You can use the example script from the linked issue, something like:

step('Go to example,com', async () => { 
  const url = "https://example.com"
  // basically any code with a template string will break Heartbeat
  await page.goto(`${url}`);
});

Create a cloud deployment in CFT against 8.13.0-SNAPSHOT version
I have created a test deployment with a custom Kibana image using this PR branch

Testing the fix

Go to your cloud testing deployment and create a browser monitor using the script from the pre-req section.
Verify that your monitor fails to run appropriately with an error like the one in the linked issue.
Go to the custom image deployment and create a monitor with the same script.
Verify that the monitor succeeds to run.
Verify that the Test Now feature (available in Detail view or on the Overview page) also works, as this is a separate route/transmission procedure.

This covers the fix for the bug. As part of this patch, we also made some changes to the ways monitors are stored/transmitted to the service. I'll add more detail in a footnote section below explaining what we did and why. The TLDR; of this, however is: we need to verify the add/edit functionality still works after these changes.

Verify edits still work

In the custom image deployment, edit your monitor. This can be done from the monitor detail view, the Synthetics management view, or the popover menu from the monitor's card on the overview page.
Modify your monitor script in some meaningful way that will cause visual changes in the test run. For example, you could add a second step to the script that throws an error.
Save your monitor, and wait for the next test run, you should be able to see your new code in the test run details in the Kibana UI. This step ensures we are generating new zip data for each persist.

Feel free to do any additional checking you might think is appropriate.

Additional testing note

I left it off the main testing instructions because it's significantly more effort to set up, but an important code path to test is for an existing, failing monitor, to be fixed by this change. To perform this test, I created a local Stack hitting a locally-running Synthetics Service. I made a monitor with template strings that would fail, and verified it broke on my local service:

Then, I swapped Kibana into this branch and verified that after its next sync, the monitor succeeded:

In this case, the monitor data is never being stored as zipped, but the pre-existing script is being zipped on the fly before the monitor is transmitted to the service.

Why and How does this work

On the Agent side of this bug, Heartbeat generates dynamic YAML when executing inline journeys. Unfortunately, there's no easy way to escape template strings users add in JS code for inline monitors, and this breaks the YAML syntax when Heartbeat tries to parse the config code.

The workaround is to use a field normally reserved for project monitors. This patch takes the user-provided script content and zips/b64-encodes it, storing that data in the project source field we normally use for a project. When we transmit the monitor data for an inline test run, Kibana now drops the plaintext monitor script and only transmits the zipped data. When Heartbeat evaluates the fields it receives, it will treat the monitor as a project and execute it in a different manner, thus un-breaking this bug.

We elected to zip the data for inline monitors in the persist stage to avoid requiring the Kibana server to handle the added burden of re-zipping all inline monitors every time it proceeds to transmit them. In the worst case this could be 250 monitors per transmission, and we'd be re-zipping and re-encoding them each time Kibana sync'd with the service.

One caveat to this change is that it is not retroactive. For any existing inline monitor that is not zipped at the time this code starts to run, the Kibana server will be required to perform the zip at transmission time, until the monitor is edited again.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

apmmachine · 2024-02-01T19:29:02Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

justinkambic · 2024-02-09T23:01:22Z

/oblt-deploy

justinkambic · 2024-02-12T17:34:43Z

/oblt-deploy

elasticmachine · 2024-02-12T20:01:01Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

x-pack/plugins/synthetics/server/routes/monitor_cruds/add_monitor.ts

x-pack/plugins/synthetics/server/routes/monitor_cruds/edit_monitor.ts

cauemarcondes

Code LGTM

vigneshshanmugam

I just checked the core zip part, added some comments. Mostly LGTM

vigneshshanmugam · 2024-02-14T23:57:33Z

x-pack/plugins/synthetics/server/synthetics_service/synthetics_service.ts

+            const locMonitors = await Promise.all(
+              bucketsByLocation[location.id].splice(0, PER_PAGE).map(async (monitorData) => {
+                // no inline script data, sync without further processing
+                if (!monitorData?.['source.inline.script']) return monitorData;


ques: why are we not setting [ConfigKey.SOURCE_INLINE]: undefined here?

Fair question - in this case there is no inline script so there's no reason to modify the monitor in any way.

This means it's either lightweight or already a project monitor, so basically we let those objects pass through unchanged.

x-pack/plugins/synthetics/server/common/mem_writable.ts