Test synthetic monitor

[ykns]

Title of this pull request?

Test playwright monitor

Small Description?

Allow a user to test a playwright synthetic monitor.

Initially tried to run playwright directly from the dashboard, however, I found that playwright doesn't play nice with webpack, see Playwright Github Issue

TODO:

• smarten up the test button UI, also consider how best to make this collapsible?
• add tests

Pull Request Checklist:

• Please make sure all jobs pass before requesting a review.
• Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if such).
• Have you lint your code locally before submission?
• Did you write tests where appropriate? Managed to get POC working and wanted to get some initial feedback while I spend some time adding necessary tests.

Related Issue?

Closes #1689

Screenshots (if appropriate):

[well-it-wasnt-me]

Please, add a better error control on the post request. Try-Catch ain't enough in this case

[well-it-wasnt-me]

Haven't check the entire codebase yet. But are you sure to substitute PROBE_SYNTHETIC_MONITOR_SCRIPT_TIMEOUT_IN_MS with SyntheticMonitorScriptTimeoutMs ?
it might be used elsewhere in the code and if so then you are gonna have 2 var with same objective

[ykns]

From what I can tell the docker-compose.base.yml takes global values and creates vars for each service

PROBE_SYNTHETIC_MONITOR_SCRIPT_TIMEOUT_IN_MS is specific to App and derived from the same global GLOBAL_PROBE_1_SYNTHETIC_MONITOR_SCRIPT_TIMEOUT_IN_MS as the other timeouts like in Probe service.

Please let me know if there's a better way to share this value across services.

[ykns]

To make things clearer, I've renamed PROBE_SYNTHETIC_MONITOR_SCRIPT_TIMEOUT_IN_MS to SYNTHETIC_MONITOR_SCRIPT_TIMEOUT_IN_MS so that it doesn't infer relationship to probe

[well-it-wasnt-me]

I'm new to the code so can't say much about it.

Just saw couple of globals with same
Purpose and raised my concern.

Anyhow. The global I was referring to was not about the yaml file but the one used in the actual code.

So. If you plan to rename an old one consult with the "elders" (aka the people who wrote this). But honestly ? Would be painless to use the old one (has more sense)

[well-it-wasnt-me]

Input validation

monitorId, script, browserTypes and screenSizeType looks like "rogue" data that can be passed without any proper check. This will open to a whole new world of security issue (and pain in the ass)

Generic Error Handling

This only covers very generic error objects. In practice, there may be different kinds of errors, including network errors, API errors, or data parsing issues, which are not specifically caught here...so what about something like this ?

catch (err) {
  if (err instanceof HTTPErrorResponse) {
    dispatch({ type: 'failure', error: new Error('API Error: ' + err.message) });
  } else if (err instanceof NetworkError) {
    dispatch({ type: 'failure', error: new Error('Network Error') });
  } else {
    dispatch({ type: 'failure', error: new Error('An unexpected error occurred') });
  }
}

[ykns]

This already runs on the probe without validation (happy to be corrected here). From what I understand a signed-in user can change these values, once verified by the UserMiddleware.getUserMiddleware.

As an improvement, the input validation can be done on the server. In the case of TestMonitorAPI.ts, the validation would look like:

• screenSizeTypes, check for only unique values that match the enumeration
• browserTypes, check for only unique values that match the enumeration

Unsure of how best to validate script without knowledge of connected systems. Is there something I'm missing?
In addition, there could be some level of debouncing around this post handler to prevent potential spamming.

Also, I don't believe monitorId needs to be checked as it is not required by SyntheticMonitor and there's a case of testing a monitor that yet to be saved.

[well-it-wasnt-me]

Again. Not yet studied the code. But I'm going on a limb here. Monitor id is important.

If they did what I think then needs to be obbligaory and validated.

Kudos on your other thoughts (Check that the other param are correct and unique)

[well-it-wasnt-me]

More info about why monitor id is important:

If they structured the application as I think...then you are gonna need a monitor to run it because of other layers implied in the mechanism.

If I'm hopefully wrong...than you are absolutely right. And monitor id is useless (as it should be).

But since is there and I don't know the code...id safely go with implementing it and check it

[well-it-wasnt-me]

All paths start with a "." (dot)

why you chosed this way of writing down the path ?

[ykns]

I moved the SyntheticMonitor to Common so that it can be shared and used by Probe and App

[well-it-wasnt-me]

Good one !

[simlarsen]

Great first stab, but we need to talk on thre architecture first before you work on this. I'll look into this PR by the end of this week and get back to you with the next steps.

[well-it-wasnt-me]

To me is all in order. Good one !

[simlarsen]

These test should run on the probe and not in the app container. So, a user can select which probe to run the tests on and it runs on that probe.

Reason: There are internal resources deployed with internal probes that are not accessible from outside, so this PR might need an update.

While you're working on this, I think we also need to create the generic architecture so any monitor type can be tested on a probe.

Please let me know if you need any more info.

[ykns]

These test should run on the probe and not in the app container. So, a user can select which probe to run the tests on and it runs on that probe.

Reason: There are internal resources deployed with internal probes that are not accessible from outside, so this PR might need an update.

While you're working on this, I think we also need to create the generic architecture so any monitor type can be tested on a probe.

Please let me know if you need any more info.

Apologies, for the delay.

For alignment here is how I believe the Probe and Ingestor work:

sequenceDiagram
  loop for each worker
  Probe ->>+ Ingestor: fetch `/monitor/list`
  Ingestor ->>+ MonitorProbeService: find monitors by probe id
  MonitorProbeService ->>- Ingestor: monitor list
  Ingestor ->>- Probe: monitor list
  loop for each monitor
  Probe ->> Probe: probe monitor
  Probe ->> Ingestor: post monitor response `/probe/response/ingest`  
  end
  end

Loading

With that in mind, my suggestion is for the Probe to call a new Ingestor endpoint monitor/test/list and post the responses back to another new Ingestor endpoint /probe/test/response/ingest. The flow would look like:

sequenceDiagram
  loop for each worker
  Probe ->>+ Ingestor: fetch `/monitor/list`
  Ingestor ->>+ MonitorProbeService: find monitors by probe id
  MonitorProbeService ->>- Ingestor: monitors
  Ingestor ->>- Probe: monitors
  loop for each monitor
  Probe ->> Probe: probe monitor
  Probe ->> Ingestor: post monitor response `/probe/response/ingest`  
  end
  Probe ->>+ Ingestor: fetch `/monitor/test/list`
  Ingestor ->>+ TestMonitorProbeService: find test monitors by probe id AND `done=false`
  TestMonitorProbeService ->>- Ingestor: test monitors
  Ingestor ->>- Probe: test monitors
  loop for each test monitor
  Probe ->> Probe: probe test monitor
  Probe ->> Ingestor: post test monitor response `/probe/test/response/ingest`  
  Ingestor ->>+ TestMonitorProbeService: update test monitors to be `done=true`
  end
  end

Loading

This keeps the test results separate from the real monitored results.