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

Clear the list of traces #59

Closed
wants to merge 2 commits into from
Closed

Conversation

lenko-d
Copy link
Contributor

@lenko-d lenko-d commented Dec 8, 2020

This PR adds the ability to clear the list of traces.

Updates: 2269

@klizhentas
Copy link
Contributor

@lenko-d how is this different from trace.Unwrap that returns you the original error?

@lenko-d
Copy link
Contributor Author

lenko-d commented Dec 8, 2020

@lenko-d how is this different from trace.Unwrap that returns you the original error?

The problem is that Unwrap changes the structure of the API's response considerably. Before Unwrap:

{
    "error": {
        "message": "method not found"
    },
    "traces": [.......]
}

After Unwrap:

{
    "message": "method not found"
}

The structural change above will most likely require changes for the clients of the API.

By using ClearTraces we eliminate the possibility of downstream changes because the structure remains the same:

{
    "error": {
        "message": "method not found"
    }
}

All I want is to simply change the web handlers like this and preserve the structure of the response JSON:

			trace.WriteError(w, trace.ClearTraces(err))

@@ -72,6 +72,16 @@ func Unwrap(err error) error {
return err
}

// ClearTraces empties the list of traces
func ClearTraces(err error) error {
Copy link
Contributor

@a-palchikov a-palchikov Dec 8, 2020

Choose a reason for hiding this comment

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

Do we need this in the trace package? So far, this is special-case functionality that works on exported attributes which can be implemented where it is required.
Another question is whether the error types trace provides should be marshaled as they're. Can we consider a way of changing that both ways - e.g. revisit the http marshalling layer that outputs these values in the expected format but obviously without the traces since they don't have them?

@lenko-d
Copy link
Contributor Author

lenko-d commented Dec 9, 2020

Closing because the issue will be handled at the marshaling layer.

@lenko-d lenko-d closed this Dec 9, 2020
@lenko-d lenko-d deleted the lenko/2269-no-stacktraces branch December 9, 2020 04:04
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.

3 participants