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

Update stacktrace to include trimmed? and use color? in web #387

Closed
wants to merge 5 commits into from
Closed

Update stacktrace to include trimmed? and use color? in web #387

wants to merge 5 commits into from

Conversation

sirmspencer
Copy link
Contributor

@sirmspencer sirmspencer commented Jan 28, 2020

  • I expanded the use of the color? option to show colors on the html response for (wrap-stacktrace-web ) based on the error type (java, clojure, etc). This uses the same logic for determining color as (wrap-stacktrace-log).

  • I also added a new tag trimmed? to use the :trimmed-elems instead of :trace-elems. :trimmed-elems is a shorter stack trace provided the error report. You can see an example in current code by comparing the log output and the web output for a :cause. The log output already uses :trimmed-elems in pst-on.

closes #386

@weavejester
Copy link
Member

Thanks for the commits. Can you just upload some screenshots of before and after so there's a record of what you're doing? It's hard to tell from the description alone.

Can you also make sure the PR adheres to the contributing guidelines, particularly the parts around commit message, changes unrelated to the purpose of the PR, and line length.

@sirmspencer
Copy link
Contributor Author

sirmspencer commented Jan 29, 2020

Default web view (existing view). The stack trace continues past the viewable screen.
image

This is the web view with color? and trimmed? = true
image

@sirmspencer
Copy link
Contributor Author

sirmspencer commented Jan 29, 2020

Default log view (existing). The stack trace continues past the visible screen.
image

Here is the log view with color? and trimmed? = true
image

@sirmspencer

This comment has been minimized.

The new parameter will display the key :trimmed-elems returned from the processes
 error instead of :trace-elems which whill show a shorter trace.
This is benificial in 1. not flooding the terminal and 2. making it easier to find
the trace directly related to the application.
Extending color? to wrap-stacktrace-web will allow the same color patterns available
in wrap-stacktrace-log.
@weavejester
Copy link
Member

I cant fit screen shots of before, the stack traces are very long.

Can you just show part of the screen? It would help to have a record of what it looks like in terms of before and after.

@weavejester
Copy link
Member

Also in terms of review, please see the contributing guidelines first. There's a bunch of formatting changes that need to be taken out, and the commit messages need to adhere to the guidelines.

@sirmspencer
Copy link
Contributor Author

sirmspencer commented Feb 7, 2020

Also in terms of review, please see the contributing guidelines first. There's a bunch of formatting changes that need to be taken out, and the commit messages need to adhere to the guidelines.

I read the guidelines and did as best as I can interpret on the commit messages, and they are updated from the initial push. You have to realize that I have never had a project with those types of guidelines, it's not obvious to me what you are asking me to change.

As far as the code, I dont see any formatting changes in the PR. Please post a review on github, I can make any changes you ask for.

Copy link
Member

@weavejester weavejester left a comment

Choose a reason for hiding this comment

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

I've highlighted the formatting changes to remove; a PR should implement a single change or feature. There are also some code style issues, and some functions that are public but no explanation of why they're public. Additions to the public API of Ring need to be justified.

ring-devel/src/ring/middleware/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/src/ring/middleware/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/src/ring/middleware/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/src/ring/middleware/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/src/ring/middleware/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/src/ring/middleware/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/src/ring/middleware/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/test/ring/middleware/test/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/test/ring/middleware/test/stacktrace.clj Outdated Show resolved Hide resolved
ring-devel/test/ring/middleware/test/stacktrace.clj Outdated Show resolved Hide resolved
@sirmspencer
Copy link
Contributor Author

sirmspencer commented Feb 10, 2020

Thank you for the review that helped me understand what you are looking for. I found a couple more formatting changes to remove after making the updates you pointed out. Sorry, I didnt see the clojure guide also had a line length. Not fixing indentation on the code I'm working on is new to me. Should I create a PR after this one to fix the indents, and other formatting?

@weavejester
Copy link
Member

Another PR to fix indentation would be fine. The reason we don't mix formatting changes with feature changes is that it muddies the diff and makes it hard to understand what has changed. If we separate out formatting changes from other changes, it's a lot easier to see which lines have changed.

This code looks like it could be written more concisely; there are a lot of functions where I think a refactor would significantly reduce the changes required. I've also briefly looked into :trimmed-elems and I don't understand why it would affect things that aren't a cause. I'm going to need to dedicate more time to this, but it's going to take at least a few weeks before I can manage that.

@sirmspencer
Copy link
Contributor Author

sirmspencer commented Feb 11, 2020

This code looks like it could be written more concisely; there are a lot of functions where I think a refactor would significantly reduce the changes required. I've also briefly looked into :trimmed-elems and I don't understand why it would affect things that aren't a cause. I'm going to need to dedicate more time to this, but it's going to take at least a few weeks before I can manage that.

You are correct that the root does not have trimmed elements, only causes do. The root :trace-elems are not very helpful because it returns the same 40+ stack trace of the application for every error, with no error details. So the function does end up setting :trace-elems to empty for the root. You can see in the screen shots, that the remaining information gets right to the point of the error.

@weavejester
Copy link
Member

weavejester commented Feb 11, 2020

Would it be better if we just made certain segments of the stacktrace collapsable? I'm also wondering if we want to test this out in an external library first, as I currently lack the time to investigate this thoroughy.

@sirmspencer
Copy link
Contributor Author

sirmspencer commented Feb 11, 2020

Would it be better if we just made certain segments of the stacktrace collapsable? I'm also wondering if we want to test this out in an external library first, as I currently lack the time to investigate this thoroughy.

I could do this, instead of 1 new option, it would be two: collapse-root? and trimmed-cause? The code would get more complex though. As written works for me, until someone requests some parts collapsible and other not, it feels like extra.

I have the code running in my current project from a local NS. How would I get it into an external library for testing?

@weavejester
Copy link
Member

You can just pull the namespace out, give it a new name, and release it as part of a new library. That will solve the problem in the short term.

In the longer term I'll get around to looking at this eventually, but I'll need to budget several days review. Currently I don't feel as though I understand the consequences of this change well enough to even begin to think about merging it, and I'm pretty sure we could make the code more concise in places.

@sirmspencer
Copy link
Contributor Author

You can just pull the namespace out, give it a new name, and release it as part of a new library. That will solve the problem in the short term.

In the longer term I'll get around to looking at this eventually, but I'll need to budget several days review. Currently I don't feel as though I understand the consequences of this change well enough to even begin to think about merging it, and I'm pretty sure we could make the code more concise in places.

Thanks for all your feedback. To help the review process, I can break out the changes for formatting, color? and trimmed?.

@sirmspencer
Copy link
Contributor Author

This PR will be split, and is no longer needed.

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.

[stacktrace.clj] Allow condensed stack trace format
2 participants