[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

…ail-to-run-migrate-to-project-monitor

elasticmachine · 2024-09-20T16:19:08Z

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

…ail-to-run-migrate-to-project-monitor

kibana-ci · 2024-09-25T18:19:06Z

💔 Build Failed

Buildkite Build
Commit: c5ace2c
Interpreting CI Failures
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-176094-c5ace2c1982e

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #41 / Synthetics API Tests AddNewMonitorsPublicAPI Browser Monitor base browser monitor
[job] [logs] FTR Configs #36 / Synthetics API Tests AddNewMonitorsPublicAPI Browser Monitor base browser monitor
[job] [logs] FTR Configs #41 / Synthetics API Tests AddNewMonitorsPublicAPI Browser Monitor base browser monitor
[job] [logs] FTR Configs #36 / Synthetics API Tests AddNewMonitorsPublicAPI Browser Monitor base browser monitor

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`synthetics`	20	25	+5

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @justinkambic

justinkambic added bug Fixes for quality problems that affect the customer experience release_note:fix Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Feb 1, 2024

justinkambic self-assigned this Feb 1, 2024

justinkambic force-pushed the 169963/inline-scripts-with-template-syntax-fail-to-run-migrate-to-project-monitor branch from e5743d5 to 668ae4c Compare February 9, 2024 23:00

justinkambic force-pushed the 169963/inline-scripts-with-template-syntax-fail-to-run-migrate-to-project-monitor branch from 668ae4c to d0f96d9 Compare February 12, 2024 19:34

justinkambic marked this pull request as ready for review February 12, 2024 20:00

justinkambic requested a review from a team as a code owner February 12, 2024 20:00

cauemarcondes reviewed Feb 13, 2024

View reviewed changes

x-pack/plugins/synthetics/server/routes/monitor_cruds/add_monitor.ts Outdated Show resolved Hide resolved

cauemarcondes reviewed Feb 13, 2024

View reviewed changes

x-pack/plugins/synthetics/server/routes/monitor_cruds/edit_monitor.ts Outdated Show resolved Hide resolved

cauemarcondes approved these changes Feb 13, 2024

View reviewed changes

justinkambic added the v8.13.0 label Feb 14, 2024

justinkambic added 7 commits February 14, 2024 18:01

PoC stage of fix.

be2a4e8

Include expect in list of imports.

9993d23

Add writable stream and zip for add/edit/sync inline monitors.

1bc597c

Fix run_once.

643ab7a

Encapsulate.

c1dadd4

PR cleanup.

15cdea6

PR feedback.

45246c2

justinkambic force-pushed the 169963/inline-scripts-with-template-syntax-fail-to-run-migrate-to-project-monitor branch from 7da0905 to 45246c2 Compare February 14, 2024 23:01

PR feedback.

48255f4

vigneshshanmugam reviewed Feb 15, 2024

View reviewed changes

justinkambic added 4 commits February 19, 2024 14:40

Simplify per PR advice.

9de233d

Improve tests, reorganize code.

ce574a9

Merge branch 'main' into 169963/inline-scripts-with-template-syntax-f…

8a599ca

…ail-to-run-migrate-to-project-monitor

Merge branch 'main' into 169963/inline-scripts-with-template-syntax-f…

b43ab54

…ail-to-run-migrate-to-project-monitor

justinkambic requested a review from a team as a code owner September 20, 2024 16:19

botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Sep 20, 2024

smith added the apm:review label Sep 20, 2024

Merge branch 'main' into 169963/inline-scripts-with-template-syntax-f…

130d1d8

…ail-to-run-migrate-to-project-monitor

justinkambic added backport:prev-major Backport to (8.x, 8.15) the previous major branch and all later branches still in development v8.16.0 and removed v8.13.0 labels Sep 23, 2024

justinkambic added 8 commits September 23, 2024 12:01

Working to clean up tests. WIP.

4473510

Fix tests.

27b5060

Merge branch 'main' into 169963/inline-scripts-with-template-syntax-f…

6b9669f

…ail-to-run-migrate-to-project-monitor

Delete unneeded file.

771b15a

Merge branch 'main' into 169963/inline-scripts-with-template-syntax-f…

5d796af

…ail-to-run-migrate-to-project-monitor

Fix project field mapping issue where monitor type is not browser.

5cbe70e

Default to whatever the inline script is if zipping should fail.

f4bc77d

Fix types.

c5ace2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

justinkambic commented Feb 1, 2024 •

edited

Loading

apmmachine commented Feb 1, 2024

justinkambic commented Feb 9, 2024

justinkambic commented Feb 12, 2024

elasticmachine commented Feb 12, 2024

cauemarcondes left a comment

vigneshshanmugam left a comment

vigneshshanmugam Feb 14, 2024

justinkambic Feb 19, 2024

elasticmachine commented Sep 20, 2024

kibana-ci commented Sep 25, 2024 •

edited

Loading

References to deprecated APIs

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

Are you sure you want to change the base?

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

Conversation

justinkambic commented Feb 1, 2024 • edited Loading

Summary

Testing instructions

Pre-requisites

Testing the fix

Verify edits still work

Additional testing note

Why and How does this work

Checklist

For maintainers

apmmachine commented Feb 1, 2024

🤖 GitHub comments

justinkambic commented Feb 9, 2024

justinkambic commented Feb 12, 2024

elasticmachine commented Feb 12, 2024

cauemarcondes left a comment

Choose a reason for hiding this comment

vigneshshanmugam left a comment

Choose a reason for hiding this comment

vigneshshanmugam Feb 14, 2024

Choose a reason for hiding this comment

justinkambic Feb 19, 2024

Choose a reason for hiding this comment

elasticmachine commented Sep 20, 2024

kibana-ci commented Sep 25, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

References to deprecated APIs

History

justinkambic commented Feb 1, 2024 •

edited

Loading

kibana-ci commented Sep 25, 2024 •

edited

Loading