Particular / ServicePulse

Production monitoring for distributed systems.
https://docs.particular.net/servicepulse/
Other
32 stars 27 forks source link

Fix incorrect logic to display error icon for Usage setup #1963

Closed johnsimons closed 1 month ago

johnsimons commented 1 month ago

The logic should be:

  1. if it is a broker, we return true if the broker connection is unsuccessful
  2. if the Audit test connection is false, we return true symbolizing that we have errors;
  3. if monitoring is enabled, we return whatever the value of the test connection is for monitoring;
  4. by default we return no errors
abparticular commented 1 month ago

I'm not sure the logic is correct. I think we only require either audit or monitoring.

The current logic seems to always require audit for non-broker transports, which is not correct.

johnsimons commented 1 month ago

We don’t require Audit, but I agree the logic is not the best. the problem is that we don’t have an api to figure out if audit is configured at all or not and hence we have to “repurpose” the test connection logic to tell us. to fix this properly we would need to figure out if the backend has audit configured or not.

abparticular commented 1 month ago

GitHub isn't letting me do a suggestion over all of the relevant code lines, but how about:

    if (useIsMonitoringEnabled()) {
      return !testResults.value?.monitoring_connection_result.connection_successful;
    }

    return !testResults.value?.audit_connection_result.connection_successful;
jpalac commented 1 month ago

It's not one or the other. If both are connected, then if either one of them has an incorrect configuration then the error should be shown.

If both are connected, both result messages should be shown and both diagnostics.

If only one ia connected then only that connected services status should be used.