Skip to content

Avoid invoking non-existent helper for system stats with Crunchy Bridge #692

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
Mar 21, 2025

Conversation

keiko713
Copy link
Contributor

Since we're running selfhosted.GetSystemState with Crunchy Bridge, the test result shows the following a bit confusing message:

	✗ System:              	error running system stats helper process: fork/exec /usr/bin/pganalyze-collector-helper: no such file or directory

This is an expected error as the helper file doesn't exist, but it is alarming, so let's not flag this error with Crunchy.

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.

Nice, thanks for fixing this.

I think this approach works (and feel free to merge as-is), but it complicates the existing Crunchy dependency on self-managed (a somewhat dubious idea in the first place), which I think makes the code harder to follow. One thing we could do instead is factor out the shared code and split out the self-managed specific part. E.g., something like

        } else if dbHost == "" || dbHost == "localhost" || dbHost == "127.0.0.1" || config.AlwaysCollectSystemData {
		systemHelperState := selfhosted.GetSystemStateFromHelper(server, logger)
		system = selfhosted.GetSystemState(server, logger, systemHelperState)
	} else {

And the Crunchy code could just pass an empty systemHelperState struct or something. These are terrible names, but I think you get the idea.

@msakrejda msakrejda changed the title Ignore helper process error with Crunchy Bridge Avoid invoking non-existent helper for system stats with Crunchy Bridge Mar 20, 2025
@msakrejda
Copy link
Contributor

I also took the liberty of updating the PR title to avoid confusion. Feel free to edit further, of course.

@keiko713
Copy link
Contributor Author

I think this approach works (and feel free to merge as-is), but it complicates the existing Crunchy dependency on self-managed (a somewhat dubious idea in the first place), which I think makes the code harder to follow. One thing we could do instead is factor out the shared code and split out the self-managed specific part. E.g., something like

Thanks for the idea! I agree that this is really a workaround (well, Crunchy using self managed one is already a workaround). I tried to extract the part that pulls data from a helper, though it didn't really click me (like you noticed, we do need to return systemHelperState (helperStatus), then we need to have a special handling of putting the values of helperStatus to state.SystemState, etc. etc.), so I'll merge this as is.
Also thanks for updating the title!

@keiko713 keiko713 merged commit 724f342 into main Mar 21, 2025
3 checks passed
@keiko713 keiko713 deleted the ignore-helper-error-with-crunchy branch March 21, 2025 02:37
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.

3 participants