jl777 / SuperNET

57 stars 222 forks source link

Better error reporting #594

Open lukechilds opened 6 years ago

lukechilds commented 6 years ago

During testing we're getting quite a few errors and it's very hard to tell what's causing them.

marketmaker will occasionally hang, drop connections or just quit. Even when we query it one by one. It's very hard for us to debug and work out what's going wrong because there's often little or no error data in the responses (or no response at all).

Also, of the responses that do report errors, many of them are inconsistent in how they indicate success/failure. For example, these are some of the variations we've came across so far:

response.status === 'active'
// or
response.result === 'success'
// or
typeof response.error === 'undefined'

Would it be possible to implement consistent error reporting across all of the commands?

jl777 commented 6 years ago

I have tried to have a consistent "result":"success" for a properly submitted api request and an "error":"error message" if an error occurred. maybe a few slipped through the cracks...

which commands return "status":"active" without a "result":"success"? I will fix that

lukechilds commented 6 years ago

I'm not sure off the top of my head but I'll keep track of them as I notice them and report back here 👍

lukechilds commented 6 years ago

Ok, so I'm getting consistent errors but realised we're on a pretty old build of mm. Wanted to test on the latest version from dev before putting time into a repro to make sure it's not already fixed.

I'm trying to build the latest version with my Docker image but builds are failing (exact same build script as before). Did something change with the build process? I can't see any changes documented.

You can view the commands used and error log here: https://hub.docker.com/r/lukechilds/barterdex-api/builds/bcesucp8e6fs8vcy5pghfhn/

jl777 commented 6 years ago

nothing changed in the build process the log shows that nanomsg is not found

lukechilds commented 6 years ago

How can you tell it's nanomsg? Are you inferring that from the

undefined reference to `nn_XXX`

errors?

Are you certain nothing changed in the build process on your end? The exact same build script worked previously: https://hub.docker.com/r/lukechilds/barterdex-api/builds/bfdhvzqpzsi9baq8ueo9psh/

I haven't made any changes to the Dockerfile.

And looking through the commit history here it looks like there have been some nanomsg related changes recently: #582 #583

Could this be related? Should we be building with ./m_mm_StaticNanoMsg now?

jl777 commented 6 years ago

probably good idea

lukechilds commented 6 years ago

@satindergrewal I can build with my Docker image from this commit: a484a9407acd755ac2c5e7923012806ccc123e97

Looks like you made quite a few changes to the build system after that and my Docker image can no longer create builds. What build process are you using now? Have you tested it on both macOS and Linux?

satindergrewal commented 6 years ago

@lukechilds I did not made change to ./m_LP which creates dynamic iguana for linux and osx. ./m_mm is updated which creates dynamic marketmaker like before for osx, but it is updated to create static marketmaker for linux.

In this process I added a new script (build_static_nanomsg.sh) based on bash shell condiment to invoke based on what OS it is being run on.

These are the PR and commits you will find helpful in which I pushed changes: https://github.com/jl777/SuperNET/pull/582/files https://github.com/jl777/SuperNET/pull/583/files https://github.com/jl777/SuperNET/pull/585/files

Hope these will help resolve your issues.

lukechilds commented 6 years ago

@satindergrewal Yeah you can see those changes appear to have broken m_mm in some environments.

Here is a successful build before the changes: https://hub.docker.com/r/lukechilds/barterdex-api/builds/bfdhvzqpzsi9baq8ueo9psh/

And here is an identical build after the changes which fails: https://hub.docker.com/r/lukechilds/barterdex-api/builds/bcesucp8e6fs8vcy5pghfhn/

Are there any extra undocumented steps I should be taking after your changes or is everything supposed to work the same as before?

./m_mm is updated which creates dynamic marketmaker like before for osx, but it is updated to create static marketmaker for linux.

Did you mean to say that the other way round? As far as I can see in this PR (#585) ./m_mm is using dynamic nanomsg for Linux and static nanomsg for macOS.

satindergrewal commented 6 years ago

oh yes, that's correct linux dymamic and mac static mm :)

the failed docker log link says this in log

[91m./m_mm: 4: ./m_mm: [[: not found ./m_mm: 8: ./m_mm: [[: not found  / tmp/cciMosPe.o: In function `LP_sockcheck':

and line 4 in m_mm is if [[ "$OSTYPE" == "linux-gnu" ]]; then

wonder if for some reason docker is complaining about this if statement.

Can you check in normal non-docker environment in a linux bash shell if you get same error or it works for you as normal?

I don't know much about docker since I never had to try or needed. Please try things first in linux environment, and report me if that fails. I think that would help troubleshoot this issue better.

lukechilds commented 6 years ago

Sure, will test that now. Also, it would probably be better to just assume the normal behaviour by default and apply macOS specific overrides inside an if statement.

For example the current if linux elseif macos build script will just silently fail on BSD or other obscure $OSTYPE values.

lukechilds commented 6 years ago

Found a couple of bugs in the build script, sent over a patch.

@jl777 @satindergrewal are you able to review this: https://github.com/jl777/SuperNET/pull/597