Skip to content
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

Inline monitor syntax errors should send errors to ES #29897

Closed
Tracked by #29692
andrewvc opened this issue Jan 18, 2022 · 3 comments · Fixed by #30013
Closed
Tracked by #29692

Inline monitor syntax errors should send errors to ES #29897

andrewvc opened this issue Jan 18, 2022 · 3 comments · Fixed by #30013
Assignees
Labels
bug Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.17.0 v8.1.0

Comments

@andrewvc
Copy link
Contributor

andrewvc commented Jan 18, 2022

Error can be reproduced by running the below monitor.

heartbeat.run_once: true
heartbeat.monitors:
- type: browser
  enabled: true
  id: my-monitor
  name: My Monitor
  screenshots: "off"
  source:
    inline:
      script:
        asdas
        step("load homepage", async () => {
            await page.goto('https://www.elastic.co');
        });
  schedule: "@every 1m"

output.console: ~
  • The synthetics runner writes the errors back to stderr without any JSON marshalling as it cannot get the registered journeys. In these cases, HB cannot write the summary event as there are no acrtive journeys.

By disabling this check, we get the summary event-

But we need to swallow the error and do it correctly.

@andrewvc andrewvc changed the title Inline monitor syntax errors should send errors to ES (needs confirmation) Inline monitor syntax errors should send errors to ES Jan 18, 2022
@andrewvc andrewvc added Team:obs-ds-hosted-services Label for the Observability Hosted Services team bug Heartbeat labels Jan 18, 2022
@lucasfcosta lucasfcosta self-assigned this Jan 24, 2022
@lucasfcosta
Copy link
Contributor

lucasfcosta commented Jan 24, 2022

As a note on this, just to clarify the ACs:

  • When we say that we need to "swallow the error and do it correctly", do we mean swallowing the error on the synthetics side and logging it as JSON? If so, should we do this for all errors in the runner?

So far, I've assumed that we will do no changes to the runner, and, instead, we'll simply keep track of the current journey despite the error and attach a summary to it when there's something logged to stderr.


Also, as I was investigating this issue, I was puzzled by the following behaviour:

  1. When running the journey directly with the runner (using the --inline param), I did see events being emitted before the SyntaxError gets printed to the console.
$ pbpaste | elastic-synthetics --screenshots off --inline --rich-events
{"type":"synthetics/metadata","@timestamp":1643021655758117,"root_fields":{"num_journeys":1,"os":{"platform":"darwin"},"package":{"name":"@elastic/synthetics","version":"1.0.0-beta.19"}},"payload":{"network_conditions":{"downloadThroughput":655360,"uploadThroughput":393216,"latency":20,"offline":false}},"package_version":"1.0.0-beta.19"}
{"type":"journey/start","@timestamp":1643021661277285,"journey":{"name":"inline","id":"inline"},"root_fields":{"os":{"platform":"darwin"},"package":{"name":"@elastic/synthetics","version":"1.0.0-beta.19"}},"payload":{"params":{},"source":"async ({ page, context, browser, params }) => {\n        scriptFn.apply(null, [core_1.step, page, context, browser, params, expect_1.expect]);\n    }"},"package_version":"1.0.0-beta.19"}
{"type":"journey/end","@timestamp":1643021661278670.2,"journey":{"name":"inline","id":"inline","status":"succeeded"},"root_fields":{"os":{"platform":"darwin"},"package":{"name":"@elastic/synthetics","version":"1.0.0-beta.19"}},"payload":{"start":7681.998327668,"end":7682.008590622,"status":"succeeded"},"package_version":"1.0.0-beta.19"}
(node:93169) UnhandledPromiseRejectionWarning: ReferenceError: asdas is not defined
[...]
  1. Considering the above, if the output of the runner was generated within Heartbeat exactly as it's generated when I run it straightaway, we wouldn't need any changes as there would be a currentJourney to which we could attach a summary. We'd have a currentJourney because journey/start is emitted before the error.
  2. When running the very same journey from within Heartbeat, it seems like the journey/start and the synthetics/metadata events are never emitted. Therefore, we don't have a current journey to which we can attach a summary.

This seems to happen because when I copy the journey from the configuration file, it preserves line-breaks, and therefore throws a ReferenceError when trying to run the journeys.

When Heartbeat runs it, however, it seems like line breaks are not present, which then trigger a SyntaxError much earlier in the runner's flow. That error happens as the runner is registering the journey, and causes it to quit. Given the runner doesn't even get to try to run journeys, it breaks straightaway without emitting a journey/start.

Considering that the line-breaks could influence the script's execution, should we look into changing that too?

@vigneshshanmugam
Copy link
Member

Thanks @lucasfcosta for adding in more details. I might have given a bad example here. Lets replace the inline script with

function(){{{{

Synthetics runner at this point cannot run any journeys and would straight away exit with status code 1 and write the errors back to stderr.

We need like a catch all error handler in the HB side that can swallow the error from Synthetics and create a monitor summary document with those context.

Considering that the line-breaks could influence the script's execution, should we look into changing that too?

I wouldn't worry too much on this one once we have a catch all handler, we don't need to deal with line-breaks in a different way. Please do correct me if i have not thought about some other scenarios.

@andrewvc
Copy link
Contributor Author

I've re-assigned this to @vigneshshanmugam since he's started work on a fix here based on a conversation we started earlier today, as well as #29505 since it will make more sense to fix them both together than separately. The reason being that both require introducing more suite concepts which will dovetail nicely with #29917 when we're ready to pull that issue.

I'm documenting some of the schema changes from that discussion here. This could warrant a new issue, but we already have an explosion of issues being handled by a single PR, so I'm OK trying to keep things lean and document this change in this comment.

In essence, the heartbeat code needs a clearer concept of when it's in the context of a suite or a monitor. At this point, we should also add the ECS event.type field in as we increase the number of types of events. We currently do have synthetics.type but we should move toward something more ECS-capable. There are basically three situations, each of which result in different documents being emitted as illustrated below:

Suite, no active journey

{
  event: {type: "console/log"},
  suite: {id: "my-monitor.id", name: "my-monitor.name"}, 
}

Suite, active journey

{
  event: {type: "console/log"},
  suite: {id: "my-monitor.id", name: "my-monitor.name"}, 
  monitor: {id: "my-monitor.id-my.journey", name: "my-monitor.name - my.journey"},
}

Inline

{
  event: {type: "console/log"},
  monitor: {id: "my-monitor.id", name: "my-monitor.name"}, 
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.17.0 v8.1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants