search

oasislabs / oasis-gateway

⛩ Developer mediated access to the Oasis Platform
Apache License 2.0
23 stars 8 forks source link

Gateway Error Refactor #229

Closed Andrew7234 closed 4 years ago

Andrew7234 commented 4 years ago

Edit** Scope: This PR refactors the errors in the backend modules used by the gateway binary.

Resolves #774

The gateway currently does not return helpful error messages or produce verbose logs. Returned error messages describe only the top-level error, such as Errorcode 1005: ErrInternalError. Please check the status of the service. This has made debugging suspected gateway bugs difficult.

This PR replaces the standard error structs with errors from github/pkg/errors. The changes allow the stacktrace and the cause of each error to be automatically collected and shown when logged.

Adds recursive causes and stack traces to error [x] Refactor errors on gateway backend modules [x] Refactor errors in http/rpc modules [x] Testing [x]

mitjat commented 4 years ago

Ping -- are there any blockers for this PR? If not, it might be worth prioritizing getting this in. The longer it sits around, the more likely merge conflicts are and the more context you lose. Also once it's merged we can all benefit form it :)

Andrew7234 commented 4 years ago

Thanks for the feedback!

Regarding the ctx traceID, the gateway currently supports logging traceIDs via the logger. Logging statements (eg logger.debug, logger.warn) take in a context struct that includes the traceID, and I confirmed that the traceIDs appear in the gateway logs. I'm not super inclined to include the traceID in the returned error message since the traceID feels more like debugging info that should be accessed by looking at the gateway logs. Once Kibana is up and running, it should make it very easy to trace the request there. Happy to hear other thoughts though.

Currently, traceIDs must be passed to the gateway in via request headers. However our gateway clients in oasis.js and go/gateway in private-parcel do not include this header, which is why traceIDs are all -1. This should be a quick fix.

codecov[bot] commented 4 years ago

Codecov Report

Merging #229 into master will decrease coverage by 0.03%. The diff coverage is 48.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
- Coverage   57.86%   57.82%   -0.04%     
==========================================
  Files          47       47              
  Lines        3579     3578       -1     
==========================================
- Hits         2071     2069       -2     
  Misses       1363     1363              
- Partials      145      146       +1
Impacted Files Coverage Δ
concurrent/conc.go 72.57% <0.00%> (ø)
rpc/http.go 70.26% <ø> (ø)
tx/wallet.go 78.57% <0.00%> (ø)
eth/client.go 23.41% <14.28%> (ø)
tx/owner.go 51.60% <50.00%> (-0.40%) :arrow_down:
backend/eth/client.go 78.18% <57.14%> (ø)
concurrent/master.go 75.81% <62.50%> (-0.09%) :arrow_down:
rpc/decoder.go 60.00% <100.00%> (ø)
rpc/encoder.go 33.33% <100.00%> (ø)

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 0e7c4f1...1712908. Read the comment docs.