Skip to content
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: more expressive errors #22448

Merged
merged 3 commits into from
Sep 13, 2021
Merged

Conversation

lesam
Copy link
Contributor

@lesam lesam commented Sep 13, 2021

Closes #22446

Instead of "internal server error", tell the client that the error is in the server logs, and actually put them there.

  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Feature flagged (if modified API)

@lesam lesam requested a review from danxmoran September 13, 2021 14:31
} else {
msg = "An internal error has occurred"
}
msg = err.Error()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep An internal error has occurred as a prefix for the returned message? Or is that just noise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even with this, the client gives:

Error: failed to backup bucket data: failed to download snapshot of shard 1: download 1: InfluxDB OSS-only command failed: 500 Internal Server Error: error creating tsm hard link: forced error for "/Users/sarnold/.influxdbv2/engine/data/b029e667ccf6262d/autogen/1/000000001-000000001.tsm"

So since the changes will always be for 500 Internal server error I'd say An internal error has occurred is just noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless we want to tag it as an error not in the expected format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like unknown error type:

@lesam lesam force-pushed the more-error-expressive branch from a581721 to 40ca5d5 Compare September 13, 2021 14:46
@lesam
Copy link
Contributor Author

lesam commented Sep 13, 2021

Fixed linter complaint

msg = err.Error()
} else {
msg = "An internal error has occurred"
msg = "An internal error has occurred - check server logs"
h.logger.Warn("internal error not returned to client", zap.Error(err))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgnorton suggested (and I agree) it is probably too insecure to return any and every error back to the client. Instead, log the error and tell the client to check the server logs - anyone with server log access should have permission to see whatever errors we create.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Particular errors that we actually want to return to the client should be converted to the correct format to be returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New error from client:

Error: failed to backup bucket data: failed to download snapshot of shard 1: InfluxDB OSS-only command failed: 500 Internal Server Error: An internal error has occurred - check server logs

From server logs:

2021-09-13T15:45:36.828559Z     warn    internal error not returned to client   {"log_id": "0WZyCYXW000", "handler": "error_logger", "error": "error creating tsm hard link: hard coded failure for /Users/sarnold/.influxdbv2/engine/data/b029e667ccf6262d/autogen/1/000000015-000000002.tsm"}

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.

Disk not supporting hard links creates weird error from cli during backup
2 participants