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

Better error reporting #594

Open
lukechilds opened this issue Feb 14, 2018 · 12 comments
Open

Better error reporting #594

lukechilds opened this issue Feb 14, 2018 · 12 comments

Comments

@lukechilds
Copy link
Contributor

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
Copy link
Owner

jl777 commented Feb 14, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

lukechilds commented Feb 17, 2018

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
Copy link
Owner

jl777 commented Feb 17, 2018

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

@lukechilds
Copy link
Contributor Author

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
Copy link
Owner

jl777 commented Feb 17, 2018

probably good idea

@lukechilds
Copy link
Contributor Author

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

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

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 �[0m �[91m/�[0m �[91mtmp/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
Copy link
Contributor Author

lukechilds commented Feb 20, 2018

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
Copy link
Contributor Author

lukechilds commented Feb 20, 2018

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

@jl777 @satindergrewal are you able to review this: #597

DeckerSU pushed a commit to DeckerSU/SuperNET that referenced this issue Nov 5, 2022
… (jl777#594)

* Call Github release API via curl to check that assets are uploaded.

* Call Github release API via curl to check that assets are uploaded.

* Call Github release API via curl to check that assets are uploaded.

* Call Github release API via curl to check that assets are uploaded.

* Call Github release API via curl to check that assets are uploaded.

* Call Github release API via curl to check that assets are uploaded.

* Call Github release API via curl to check that assets are uploaded.

* Log DEBUG_UPLOADED and RELEASE_UPLOADED.

* Use COMMIT_TAG from env to determine DEBUG_UPLOADED and RELEASE_UPLOADED.

* Use TAG from env to determine DEBUG_UPLOADED and RELEASE_UPLOADED.

* Build and publish to Github only when corresponding binaries are not uploaded.

* Fix bash if on Setup ENV.

* Check the git tag instead of hard code.

* Clean upload dir -> Recreate upload dir.

* If commit is tagged edit existing release, create new release if there's no tag.

* Always echo DEBUG_UPLOADED, RELEASE_UPLOADED and RELEASE_TAG.

* Try to fix RELEASE_TAG when commit is not tagged.

* echo $(Build.BuildId)

* Echo if commit tag is empty.

* Use [ -z $TAG ] to determine if $TAG is empty.

* Enable the Build and test Debug stage back.

* Release only on mm2 branch again.
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

No branches or pull requests

3 participants