-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix(core): small changes to diagnostics #11563
Conversation
Doesn't messaging already do this with |
@samuelmasse The diagnostic report takes data from various places to give a clear picture of the client's situation. It's a summary of its installation. And since we don't have messaging logs steamed to the admin, in the log file or in the db, it's necessary to consolidate the information there because we don't often have access to the infra guy who can check the stdout |
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
this.logger.info(`NLU server manually handled at: ${endpoint}`) | ||
process.NLU_ENDPOINT = endpoint | ||
} |
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.
Why does this need to be added here?
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 be effective the diag must avoid using any of botpress' internals and reflect the true state of the app. The nlu endpoint can come from the module config, or from 2 env variables, so there's no way to be 100% sure of what the effective value is (and I cant use the ghost to read the config in the db)
@allardy can we merge this? |
If no endpoint is specified, it will try to connect on different hosts:
This could help determine connection issues in various settings (eg: restrictions on a specific hostname, etc).
@franklevasseur @samuelmasse @laurentlp Can you check if the logic I use there is correct, and do you think something else can be added to check the overall health of everything?