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.

Sign up for GitHub

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

Jump to bottom

Implement verbose error page mode #368

Closed
wants to merge 2 commits into from
Closed

Implement verbose error page mode #368

wants to merge 2 commits into from

Conversation

sveneberth
Copy link
Member

Display several meta information of the current request, session and environment on the error page for faster debugging.
Can be enabled by setting conf[viur.debug.verboseErrorHandler"] to True.

I changed the error.css a bit to support larger content and the new content. Because afaik a new design is already in the pipeline, this is only very rudimentary.

Screenshot

@sveneberth
Implement verbose error page mode
87edf3f 
Display several meta information of the current request, session and
environment on the error page for faster debugging.
Can be enabled by setting `conf[viur.debug.verboseErrorHandler"]` to `True`.

I changed the error.css a bit to support larger content and the new
content.
@sveneberth sveneberth added feature New feature or request Priority: Low This issue can be considered with enough idle time. labels Mar 2, 2022
sveneberth
sveneberth commented Mar 2, 2022
View reviewed changes
core/request.py
debugInformation = ""
# We're running on development server or verbose mode is on
if self.isDevServer or conf["viur.debug.verboseErrorHandler"]:
if not isinstance(e, errors.HTTPException):
Copy link
Member Author

Choose a reason for hiding this comment

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

@viur-framework/developers What do your think about this. Shall we print a traceback on HTTPExceptions or not? Might be helpful to find the right case, if there are multiple reasons.

@tsteinruecken
Copy link
Contributor

I'm undecided about this one. I generally like the idea of a better error page, however i see a few problems with this approach.

  1. Remove that config switch. Make this the default for local development and keep it disabled if deployed. Someone will forget to turn this switch back of when deploying - causing probably sensitive information to leak.
  2. None of the information added has any additional value for the developer. You really know the state of your session, the parameters supplied, your python version and so on (or can very easily retrieve these information from your browser / environment etc). But what we could do is collecting all the logs that have been generated by that request that lead to the exception (at least the ViUR ones) - because this information is usually swamped out by all the logs the dev_appserver / google api calls generated.
  3. Ignore httpErrors - unless traceExceptions is set. That flag is explicitly for debugging these.

@phorward phorward mentioned this pull request Mar 18, 2022
Open
@phorward phorward changed the base branch from develop to main April 27, 2022 09:54
@phorward
Copy link
Member

Hello @sveneberth, I'm in the opinion this feature is useful and should be integrated somehow. Can you please update your pull request to current main, or provide a similar solution in viur-base?

@phorward phorward changed the base branch from main to develop April 27, 2022 11:30
phorward
phorward requested changes Apr 27, 2022
View reviewed changes
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Please see previous comment.

@phorward phorward removed the Priority: Low This issue can be considered with enough idle time. label Apr 27, 2022
@sveneberth sveneberth self-assigned this Apr 27, 2022
@phorward
Copy link
Member

As there is no more interaction here, the ticket will be closed.
It cannot be merged in this way, but a verbose and nice error reporting would be great.
Please see my comments on this in here and create a different approach based on the work done here. Thanks.

@phorward phorward closed this Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phorward phorward phorward requested changes

@tsteinruecken tsteinruecken Awaiting requested review from tsteinruecken

@skoegl skoegl Awaiting requested review from skoegl

@akelch akelch Awaiting requested review from akelch

Assignees

@sveneberth sveneberth

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sveneberth @tsteinruecken @phorward