Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Piti/feat/trace on dev test #2358

Closed
wants to merge 3 commits into from

Conversation

piti118
Copy link

@piti118 piti118 commented Dec 30, 2022

What is being done in this PR?

This PR fixes issue #1904. And fix the bug caused by one of the previous pr that trace will only appear on development but not test environment.

What are the main choices made to get to this solution?

Without traces it's much harder to debug. The doc is also misleading that we will get a trace but in fact we didn't. This PR fix that issue.

https://gobuffalo.io/documentation/request_handling/errors/#default-error-handling-development

List the manual test cases you've covered before sending this PR:

The tests are covered in unit tests.

On dev, test: when handler return error being (pkg/errors or context.Error()) if it contains stack trace then it should return that stack trace.
On other env, trace is hidden.

@piti118 piti118 requested a review from a team as a code owner December 30, 2022 10:35
Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

First, even though it's not a concrete rule, it could be better if we don't rely on specific third-party packages as much as possible and we previously removed usages of pkg/errors before. See old PRs to check the history of tracing features.

I mostly agree that having a stack trace is better for debugging the user's application but supporting it from the perspective of the framework could be made with a different approach without directly supporting a specific third party. I was working on the related code before but could not make it a high priority before. It would be better if it is flexible enough, I am also considering better support of events, and I will work on it soon.

Related PRs:
[1] #138
[2] #1643
[3] #1930
[4] #2352

env := c.Value("env")
return env != nil && env.(string) != "development" && env.(string) != "test"
Copy link
Member

Choose a reason for hiding this comment

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

the environment "test" should execute the same code set as "production" (with data for testing) and this is not acceptable.

@sio4 sio4 self-assigned this Jan 21, 2023
@sio4 sio4 added the wontfix This will not be worked on label Jan 21, 2023
@sio4
Copy link
Member

sio4 commented Jan 22, 2023

Hi @piti118,
Could you please check if PR #2361 covers your needs for stack trace? It will print out stack trace when if the user's handler returns an error that supports %+v like the error generated with pkg/errors.

@sio4
Copy link
Member

sio4 commented Jan 24, 2023

Hi, please check #2361 for the current status. I just merged the PR into the v1 branch and I am going to close this PR to proceed to the next release.

@sio4 sio4 closed this Jan 24, 2023
@sio4 sio4 added this to the v1.1.0 milestone Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
s: obsolete wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants