Misleading error when generating a report with no deployment #846

kczpl · 2024-11-11T12:43:13Z

Summary

The problem is that when running local tests, the report generator errors out with a misleading message instead of explicitly stating that there is no deployment and therefore cannot generate a report. The error message should be clearer.

This pull request changes the error message when there is no ongoing deployment while generating a report. It improves error handling by checking whether the current Terraform state includes the metrics instance and errors out when it doesn't. To implement this, the ExternalDBSettings.DriverName configuration was changed to use PostgreSQL by default.

Ticket Link

--> https://mattermost.atlassian.net/browse/MM-56949
Github issue: mattermost/mattermost#29148

agarciamontoro · 2024-11-11T12:45:11Z

Thank you for the PR, @kczpl! Will take a look as soon as I can :)

kczpl · 2024-11-11T12:55:45Z

Thank you! I've also responded in the contributors channel thread.

If my understanding of the issue or approach is incorrect, I'm ready to continue working on this, as I'm still unsure about my solution. :))

agarciamontoro

Thanks, @kczpl! The code looks good, just a couple of comments:

If no deployer.json is configured at all, there is another error that is returned earlier. I'm thinking that we can fix this by setting the default of TerraformStateDir as ./ltstatus (I don't really know why I didn't do that when I added it, to be honest). The error is the following:

> go run ./cmd/ltctl report generate --output=base.out --label=base "2022-10-20 08:43:39" "2022-10-20 08:44:53"
Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server...Error: failed to create terraform engine: not enough permissions to create Terraform state directory "/var/lib/mattermost-load-test-ng".
Here's some alternatives you can try:
	1. Change the TerraformStateDir setting in config/deployer.json to a directory you have permissions over (recommended).
	2. Manually create the currently configured directory "/var/lib/mattermost-load-test-ng" and change its owner to your current user.
	3. Run this and all next commands as root (not recommended).
exit status 1

It seems we're missing a newline when we return the Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server... error, so the new error is a bit buried. Can you add it? Here's what I see:

> go run ./cmd/ltctl report generate --output=base.out --label=base "2022-10-20 08:43:39" "2022-10-20 08:44:53"
Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server...Error: the output has no metrics, no active deployment found
exit status 1

agarciamontoro · 2024-11-11T17:26:03Z

cmd/ltctl/report.go

@@ -90,6 +90,11 @@ func RunGenerateReportCmdF(cmd *cobra.Command, args []string) error {
 		if err != nil {
 			return fmt.Errorf("could not parse output: %w", err)
 		}
+
+		if !output.HasMetrics() {
+			return fmt.Errorf("the output has no metrics, no active deployment found")


Here we can return something slightly more useful to the user, in the lines of:

Suggested change

return fmt.Errorf("the output has no metrics, no active deployment found")

return fmt.Errorf("no active deployment found, use the --prometheus-url flag if you have a local or manually deployed Prometheus server running")

sure thing! Added that with an extra \n for better readability.

Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server... Error: no active deployment found, use the `--prometheus-url` flag. If you have a local or manually deployed Prometheus server running exit status 1

If no deployer.json is configured at all, there is another error that is returned earlier. I'm thinking that we can fix this by setting the default of TerraformStateDir as ./ltstatus (I don't really know why I didn't do that when I added it, to be honest). The error is the following:

I get now - I thought it was intentional and that the error occurred because I had no state provided. I have changed the default as suggested and added a small test.

It seems we're missing a newline when we return the Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server... error, so the new error is a bit buried. Can you add it?

Not a problem! I agree it's not user-friendly, and I've fixed that

TerraformStateDir default to ./ltstate

agnivade · 2024-11-15T06:56:07Z

cmd/ltctl/report.go

@@ -90,6 +90,11 @@ func RunGenerateReportCmdF(cmd *cobra.Command, args []string) error {
 		if err != nil {
 			return fmt.Errorf("could not parse output: %w", err)
 		}
+
+		if !output.HasMetrics() {
+			return fmt.Errorf("no active deployment found, use the `--prometheus-url` flag.\nIf you have a local or manually deployed Prometheus server running")


No need to break the line. It's a single error message. Let the user's console break the line for us :)

Makes sense :)) I've just pushed the removal

agnivade · 2024-11-15T06:56:51Z

deployment/config.go

@@ -93,7 +93,8 @@ type Config struct {
 	Report                report.Config
 	// Directory under which the .terraform directory and state files are managed.
 	// It will be created if it does not exist
-	TerraformStateDir string `default:"/var/lib/mattermost-load-test-ng" validate:"notempty"`
+	// ./ltstate is the default value used when `deployer.json`` is configured at all
+	TerraformStateDir string `default:"./ltstate" validate:"notempty"`


Fine with the change, but is this somehow related to the PR?

hi @agnivade ! :D
I did this so we can reach !output.HasMetrics() in report.go.
As far as I understand, we want to change the default directory, because when deployer.json is not configured it takes default path and we get a 'no permission' error instead no deployment

This is what @agarciamontoro mentioned in this comment.

@agnivade: Not the original goal, but to get to the correct state we wanted, we need it.

Ah, understood!

agarciamontoro

Just a couple of nits in the comments, but other than that it looks good!

cmd/ltctl/report.go

deployment/config.go

Co-authored-by: Alejandro García Montoro <[email protected]>

agnivade

Nicely done @kczpl !

agarciamontoro

Thank you, @kczpl, this is great! I've already tested it, so we can get this merged :)

kczpl · 2024-11-18T19:04:54Z

@agarciamontoro @agnivade
thanks guys! I'm looking forward to more open issues in this repo

kczpl added 2 commits November 10, 2024 17:05

Fix inconsistent error while generating report with no deployment

690ac4a

Merge branch 'mattermost:master' into fix/no-deployment-misleading-error

374433e

agarciamontoro requested review from agnivade and agarciamontoro November 11, 2024 12:44

agarciamontoro added the 2: Dev Review Requires review by a core committer label Nov 11, 2024

agarciamontoro requested changes Nov 11, 2024

View reviewed changes

Improve error message while no active deployment found and change

a4a3c29

TerraformStateDir default to ./ltstate

kczpl requested a review from agarciamontoro November 13, 2024 09:10

agnivade reviewed Nov 15, 2024

View reviewed changes

Remove unnecessary new line on no metrics error in report generation

513551c

agarciamontoro requested changes Nov 15, 2024

View reviewed changes

cmd/ltctl/report.go Outdated Show resolved Hide resolved

deployment/config.go Outdated Show resolved Hide resolved

kczpl and others added 2 commits November 15, 2024 12:02

Remove comment in config.go

8bec619

Co-authored-by: Alejandro García Montoro <[email protected]>

Update no deployment message in report generation

cc5a7f6

Co-authored-by: Alejandro García Montoro <[email protected]>

agnivade approved these changes Nov 15, 2024

View reviewed changes

agarciamontoro self-requested a review November 18, 2024 15:53

agarciamontoro approved these changes Nov 18, 2024

View reviewed changes

agarciamontoro added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Nov 18, 2024

agarciamontoro merged commit 286a3f7 into mattermost:master Nov 18, 2024
1 check passed

kczpl mentioned this pull request Nov 19, 2024

Load-test: Trying to generate a report with no deployment throws a misleading error mattermost/mattermost#29148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misleading error when generating a report with no deployment #846

Misleading error when generating a report with no deployment #846

kczpl commented Nov 11, 2024

agarciamontoro commented Nov 11, 2024

kczpl commented Nov 11, 2024

agarciamontoro left a comment •

edited

Loading

agarciamontoro Nov 11, 2024

kczpl Nov 12, 2024 •

edited

Loading

kczpl Nov 12, 2024

agnivade Nov 15, 2024

kczpl Nov 15, 2024

agnivade Nov 15, 2024

kczpl Nov 15, 2024

agarciamontoro Nov 15, 2024

agnivade Nov 15, 2024

agarciamontoro left a comment

agnivade left a comment

agarciamontoro left a comment

kczpl commented Nov 18, 2024

	return fmt.Errorf("the output has no metrics, no active deployment found")
	return fmt.Errorf("no active deployment found, use the --prometheus-url flag if you have a local or manually deployed Prometheus server running")

Misleading error when generating a report with no deployment #846

Misleading error when generating a report with no deployment #846

Conversation

kczpl commented Nov 11, 2024

Summary

Ticket Link

agarciamontoro commented Nov 11, 2024

kczpl commented Nov 11, 2024

agarciamontoro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kczpl Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

agnivade left a comment

Choose a reason for hiding this comment

agarciamontoro left a comment

Choose a reason for hiding this comment

kczpl commented Nov 18, 2024

agarciamontoro left a comment •

edited

Loading

kczpl Nov 12, 2024 •

edited

Loading