Add WithStack to Client calls to split individual errors in Sentry

[aslakknutsen]

No description provided.

[alien-ike]

Ike Plugins (test-keeper)

Thank you @aslakknutsen for this contribution!

It appears that no tests have been added or updated in this PR.

Automated tests give us confidence in shipping reliable software. Please add some as part of this change.

If you are an admin or the reviewer of this PR and you are sure that no test is needed then you can use the command /ok-without-tests as a comment to make the status green.

Your plugin configuration is stored in the file.

[fabric8cd]

@aslakknutsen snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-2066-1

[codecov-io]

Codecov Report

Merging #2066 into master will not change coverage.
The diff coverage is 2.27%.

@@           Coverage Diff           @@
##           master    #2066   +/-   ##
=======================================
  Coverage   67.18%   67.18%           
=======================================
  Files         157      157           
  Lines       14189    14189           
=======================================
  Hits         9533     9533           
  Misses       3760     3760           
  Partials      896      896

Impacted Files	Coverage Δ
account/init_tenant.go	0% <0%> (ø)	⬆️
codebase/che/client.go	14.55% <3.12%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d64ec3...adab569. Read the comment docs.

[surajssd]

@aslakknutsen what is the best practice around using WithStack? When do we use it? Should we be using it only in the controller methods? Or something like that?

Because IIUC controller methods when they return the error is directly a user facing one, do we want to show stack traces there?

[aslakknutsen]

@surajssd I think the reason why Sentry is getting a bit confused is that there are certain places we 'move an err' untouched all the way out. This could happen especially around 'external calls to other libs/clients' etc that does not use stacks. Showing the stack to the user is a different story, that needs to be filtered regardless in a level above.

Assuming that this experiment works; I would argue we should start wrapping at the first point where we know it's not wrapped? errors from go core, pg, http clients etc

[stooke]

Out of curiosity, I did some experimenting. I was curious to see what would happen if a WithStack error was wrapped. Turns out you end up with both stack traces, so at least information isn't lost. It would be great if WithStack() and Wrap() checked and only added a stack if none present. (I have a sample implementation but it uses reflection because it lives on top of the existing pkg/errors code).

[aslakknutsen]

@stooke maybe something to bring upstream?

[stooke]

@aslakknutsen, I was thinking about submitting it, but I need to do some thinking on the impact of reflection in Go - my current implementation tests for the type, and that can be inefficient (although it's the error path, so probably not really an issue)