-
Notifications
You must be signed in to change notification settings - Fork 232
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
Test synthetic monitor #1697
Test synthetic monitor #1697
Conversation
c03d171
to
ca8277d
Compare
Common/Server/API/TestMonitorAPI.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add a better error control on the post request. Try-Catch ain't enough in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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') });
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 enumerationbrowserTypes
, 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -6,7 +6,7 @@ import CustomCodeMonitor from "./MonitorTypes/CustomCodeMonitor"; | |||
import PingMonitor, { PingResponse } from "./MonitorTypes/PingMonitor"; | |||
import PortMonitor, { PortMonitorResponse } from "./MonitorTypes/PortMonitor"; | |||
import SSLMonitor, { SslResponse } from "./MonitorTypes/SslMonitor"; | |||
import SyntheticMonitor from "./MonitorTypes/SyntheticMonitor"; | |||
import SyntheticMonitor from "Common/Utils/Monitors/MonitorTypes/SyntheticMonitor"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All paths start with a "." (dot)
why you chosed this way of writing down the path ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the SyntheticMonitor to Common
so that it can be shared and used by Probe
and App
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one !
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. |
…ynthentic monitors
It would require heavy changes to the dockerfile, something like [this](https://github.com/fivemru/playwright-docker-alpine)
…n sythentic monitors
ca8277d
to
b2110e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me is all in order. Good one !
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 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
With that in mind, my suggestion is for the 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
This keeps the test results separate from the real monitored results. |
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:
Pull Request Checklist:
closes #XXXX
in your comment to auto-close the issue that your PR fixes (if such).Related Issue?
Closes #1689
Screenshots (if appropriate):