-
Notifications
You must be signed in to change notification settings - Fork 42
feat: use new kibana api for pushing monitors #649
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
feat: use new kibana api for pushing monitors #649
Conversation
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 just tested this in cloud, and I noticed that none of the monitors pushed via this branch ran on the service, and there was no feedback as to why.
This did not happen on main for the same monitor configuration.
I know this is still in progress, but requesting changes just to make sure we re-test that the monitors are actually running on the service and private locations.
I tested this with a sample payload from the existing agent passed to the new api, and verified that the monitor executed on the service and returned back up.
76e05b0
to
92b6c27
Compare
When you first use the v2 api for the first time, monitors are marked as added instead of updated. I think we should adjust the logic so that if the monitors are returned from the You can easily recreate this issue btw, even if using the new api with a fresh cluster, by pushing a batch of monitors, disabling one monitor from the UI, and repushing. The monitor will be marked as |
When changing monitors to When changing
Also, something more confusing is happening with
When setting
Notice how monitor |
Fixed 7e1a505
We don't support those under
{
params: {},
playwrightOptions: {},
monitor: {. } // Not supported under here.
}
Good that you brought up these concerns, Let move the discussion here #657 (comment) and unblock this PR.
I am not able to reproduce it, Tried with different combinations. Would like to understand more if I have missed something. Lets sync and address it. |
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.
Smoke tested !!
By adding few hundred monitors !!
By deleting few of them !!
By editing few from code and ui !!
Things seems to be working as expected !!
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.
(node:64665) ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
⚠ If you are using lightweight monitors with less than 1 minute resolution, these will be saved to the nearest supported frequency
> Pushing monitors for project: prod_overview_monitors
> bundling 1 monitors
✖ Erroring or updating 1 monitors (502ms)
> Failed to save or update monitor. Configuration is not valid: monitor(production-logging-aws-af-south-1)
Invalid value "undefined" supplied to "content"
it's failing for me on 8.5 with this error
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 can't recreate the error that Shahzad is seeing in 8.5, but I do have this error
In 8.5.2, we did not support hash and so it's hitting our error handling. Is there any way we can omit it hash when pushing via the legacy flow? Example heartbeat.yml
|
also broken for 8.4.0
|
This happens if you are trying to push Lightweight monitors prior to 8.5 as all are treated as Browser monitors.
Fixed. |
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.
LGTM !!
I think timer could be adjusted to be in seconds insead of mili !!
Testing
npm init <dir>
command.npm link
.