-
Notifications
You must be signed in to change notification settings - Fork 482
Move health check to the backed #2120
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
base: main
Are you sure you want to change the base?
Conversation
| */ | ||
| async testDatasource() { | ||
| const backendDS = new DataSourceWithBackend(this.instanceSettings); | ||
| try { |
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.
For this, I am using this approach for now, but it should be temporary, I have a separate issue to track migrating from DatasourceApi to DatasourceWithBackend when this is complete we will no longer need to have this here.
itsmylife
left a comment
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.
It looks good but we need to double check if we still need testDatasource()
| /** | ||
| * Test connection to Zabbix API and external history DB. | ||
| */ | ||
| async testDatasource() { |
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.
Do we still need this method? I think this is needed for frontend implementation. Now the config editor must have called backend CheckHealth endpoint out of the box.
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 thought we still needed it. Maybe once we migrate to DatasourceWithBackend (in progress) we don't, but even then, for this scenario we will still need it, because the part that tests the DB connection cannot be moved to the backend, in the frontend we call those datasource's "testDatasource" and they call their backend, and so here we merge the results from both.
I also checked a few other datasources and can see that they are still using the testDatasource method, for example: jaeger and tempo
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.
They were frontend only data sources and migrated to backend. Actually I raised that question in some other place too. In prometheus we don't use it. Do you mind checking what happens when we remove that method?
This PR moves the health check to backend only leaving in the frontend the functionality to test the dbconnector datasource.
Leaving the
dbconnector.testDataSourceshould be fine since the datasource types we allow for db connection with Zabbix already are backend datasources, and so their health requests would go through the backend.Verified:
Clicking test and seeing a
healthrequest go out.IMPORTANT: While testing this in the UI, I found a bug with the config editor - whenever a change is made in the UI and tested, the changes don't take effect (i.e. disabling trends, keeps
trendsset totrue, enabling db connection keepdbConnectionEnabledset tofalseand so on.). Created a separate issue to fix thisFixes: #124