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

chore: add logging for dashboards/get warnings #30365

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Sep 24, 2024

SUMMARY

We're currently not logging flask warnings to the logger as part of the "catch all" @safe or @handle_api_exception decorators. I think @handle_api_exception does a more thorough job of catching different errors and handling, logging, raising, etc. We're applying @handle_api_exception on most base restapi functions, but using @safe on the adhoc apis. Maybe we can look at combining these and only having one decorator. I also removed the exception handling in the api because they will be caught and handled in the decorator.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@github-actions github-actions bot added the api Related to the REST API label Sep 24, 2024
@eschutho eschutho marked this pull request as ready for review September 25, 2024 19:22
@dosubot dosubot bot added the logging Creates a UI or API endpoint that could benefit from logging. label Sep 25, 2024
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Looks good! I think we need to get rid of @safe at some point, since the schema of the error payload it produces are not SIP-41 compliant.

@eschutho eschutho changed the title chore: add logging for error handling apis chore: add logging for dashboards/get warnings Sep 27, 2024
@sadpandajoe sadpandajoe merged commit 96b0bcf into master Sep 27, 2024
64 of 66 checks passed
@sadpandajoe sadpandajoe deleted the elizabeth/api-error-handling branch September 27, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API logging Creates a UI or API endpoint that could benefit from logging. preset-io size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants