Skip to content

Update messages when the monitoring user doesn't have a proper setup #699

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

Merged
merged 2 commits into from
Apr 9, 2025

Conversation

keiko713
Copy link
Contributor

@keiko713 keiko713 commented Apr 8, 2025

Previously, when the monitoring user is neither superuser nor has pg_monitor role, we were showing error messages saying that please use a superuser or create a helper function. While this is true for Aiven, especially for "creating a helper function" guidance, it's not true for other cases and when the user hits this with other providers, the error message is confusing and make it not clear which actions to take. Update the messaging to make it clear the next action for these cases.

This part, to link the Aiven doc was updated in #532 and I did say "it's okay to use Aiven link", but we had some report from the other provider's customer who was confused by this error, so I think it's better to avoid showing the link.
https://github.com/pganalyze/collector/pull/532/files#r1566602511

…oper setup

Previously, when the monitoring user is neither superuser nor has
pg_monitor role, we were showing error messages saying that please use a
superuser or create a helper function. While this is true for Aiven,
especially for "creating a helper function" guidance, it's not true for
other cases and when the user hits this with other providers, the error
message is confusing and make it not clear which actions to take.
Update the messaging to make it clear the next action for these cases.
@keiko713 keiko713 requested a review from a team April 8, 2025 06:19
Copy link
Contributor

@msakrejda msakrejda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a separate troubleshooting section for this to our docs instead?

@keiko713
Copy link
Contributor Author

keiko713 commented Apr 9, 2025

Uhmm, I think there is not much to troubleshoot honestly. The messaging here is clear, the monitoring user is neither a superuser or a user with the pg_monitor role - so the fix is to use a superuser or grant pg_monitor to the monitoring user.
(except Aiven, which we do have a doc and we will show the link)

Copy link
Contributor

@msakrejda msakrejda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reviewing the actual changes, you're absolutely right.

"You are not connecting as a user with the pg_monitor role or a superuser." +
" Please make sure the monitoring user used by the collector has been granted the pg_monitor role or is a superuser.")
if c.Config.SystemType == "aiven" {
c.Logger.PrintInfo("For aiven, you can also set up the monitoring helper functions (https://pganalyze.com/docs/install/aiven/01_create_monitoring_user).")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set up

Nice catch 😄

"You are not connecting as a user with the pg_monitor role or a superuser." +
" Please make sure the monitoring user used by the collector has been granted the pg_monitor role or is a superuser.")
if c.Config.SystemType == "aiven" {
c.Logger.PrintInfo("For aiven, you can also set up the monitoring helper functions (https://pganalyze.com/docs/install/aiven/01_create_monitoring_user).")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also going to say that their branding uses an uppercase A, but after a quick fact-check of that impression, their branding uses both versions liberally, so this is fine 🤷‍♂️.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, well let's use Aiven, thanks for pointing out!

c.SelfTest.HintCollectionAspect(state.CollectionAspectPgStatStatements, "Please make sure the monitoring user used by the collector has been granted the pg_monitor role or is a superuser.")
c.Logger.PrintInfo("Warning: Monitoring user may have insufficient permissions to capture all queries.\n" +
"You are not connecting as a superuser." +
" Please make sure the monitoring user used by the collector has been granted the pg_monitor role or is a superuser, to get query statistics for all roles.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
" Please make sure the monitoring user used by the collector has been granted the pg_monitor role or is a superuser, to get query statistics for all roles.")
" Please make sure the monitoring user used by the collector has been granted the pg_monitor role or is a superuser in order to get query statistics for all roles.")

This is kind of a long sentence, and I think adding "in order" makes it easier to interpret. "To" by itself has many possible meanings, making this harder to parse.

@keiko713 keiko713 merged commit 126d69d into main Apr 9, 2025
3 checks passed
@keiko713 keiko713 deleted the update-aiven-warning branch April 9, 2025 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants