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

feat: ability to retrieve logs #478

Merged
merged 20 commits into from
Sep 9, 2022
Merged

feat: ability to retrieve logs #478

merged 20 commits into from
Sep 9, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Aug 22, 2022

Closes #469.

First iteration of "feat: ability to retrieve logs #469".
This PR uses features implemented in joinmarket-webui/jam-docker#51 and must be tested in conjunction with it.

Features:

  • ability to retrieve logs
  • refresh log file contents
  • auto scroll to bottom on refresh
  • colorize lines by log level (reverted: uses too much memory)

Possible improvements for the next iteration:

  • auto refresh (aka "follow log output")
  • filter by log level
  • colorize lines by log level

Copying a phrase from the other PR:

As always, inputs are highly appreciated and especially welcomed on this PR, as it contains rather impactful changes.
If you have any idea how to do things better or in a smarter way, please let me know!

Also, if you have any questions or are unsure about anything - don't hold back! Happy to answer and discuss any upcoming doubts/thoughts/ideas.

OLD How to test (outdated: see below)

  1. Rebuild the docker image for the secondary regtest container
docker build --label "local" \
      --build-arg JAM_REPO_REF=master \
      --build-arg JM_SERVER_REPO_REF=master \
      --tag "joinmarket-webui/jam-standalone-logs-local" ./standalone
  1. Change the secondary docker image to use the newly built image
diff --git a/docker/regtest/dockerfile-deps/joinmarket/webui-standalone/Dockerfile b/docker/regtest/dockerfile-deps/joinmarket/webui-standalone/Dockerfile
index 84a1e68..f8a8ec4 100644
--- a/docker/regtest/dockerfile-deps/joinmarket/webui-standalone/Dockerfile
+++ b/docker/regtest/dockerfile-deps/joinmarket/webui-standalone/Dockerfile
@@ -1,4 +1,5 @@
-FROM ghcr.io/joinmarket-webui/jam-dev-standalone:master
+#FROM ghcr.io/joinmarket-webui/jam-dev-standalone:master
+FROM joinmarket-webui/jam-standalone-logs-local 
  1. Rebuild your local regtest setup, do not use npm run regtest:rebuild, but:
npm run regtest:clear && docker-compose \
        --env-file docker/regtest/.env.example \
        --file docker/regtest/docker-compose.yml build

(why? -> because --pull is not supported if you have a locally built image in your docker-compose setup)

  1. start the regtest env as normal, but instead of npm start, run:
npm run start:secondary

How to test (up-to-date)

  1. Rebuild your regtest env npm run regtest:rebuild
  2. Start Jam with npm run start:regtest:secondary

📸

📸 reverted changes (just for reference; might re-implement in future iterations)

@theborakompanioni theborakompanioni added enhancement New feature or request concept Wild idea, or too many details unknown yet labels Aug 22, 2022
@theborakompanioni theborakompanioni self-assigned this Aug 22, 2022
@theborakompanioni theborakompanioni marked this pull request as ready for review August 23, 2022 11:04
Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

Nicely done! Tested locally on regtest, and everything worked.

Not sure about the primary/secondary naming, as mentioned, but maybe I'm misunderstanding things.

src/libs/JamApi.ts Show resolved Hide resolved
src/libs/JmWalletApi.ts Show resolved Hide resolved
ws: true,
})
)
const SECONDARY_CONTAINER_JAM_API_PORT = 29080
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the PRIMARY & SECONDARY naming. Shouldn't it be standalone and ui-only?

The primary/secondary distinction is for our regtest setup only, or is it not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You've hit a real pain point.

First off, I totally agree - ui-only and standalone is already way better naming.
primary and secondary is just too tightly coupled to the regtest env and the docs mention npm start as an alternative to running the docker images.

However, ui-only and standalone are the terminologies to different the docker distributions.
In this case, "secondary" is really using a standalone docker image as backend. But "primary" does not use ui-only, it just "mimics" it with setupProxy. This might lead to confusion (at least it is already confusing to me to some extent).

Just for the record:
Imho, the correct approach would be, instead of advocating using npm start to access a mainnet jm instance,
building Jam (with npm build) and delivering it via a real web server. But that seems overly complicated for "ordinary" users - that's why the ui-only docker image is so handy - it does this out-of-the-box.

Having said that - it is nice that npm start also works as a quick way to start Jam and we should aim to keep it that way.

Thanks for your input 🧡
I'll try to come up with a more suitable approach.

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Aug 24, 2022

Not sure about the primary/secondary naming, as mentioned, but maybe I'm misunderstanding things.

Introduced "backend types" native and jam-standalone in the setupProxy script.
Everything can be controlled by environment variables now, with ports defaulting to jm's standard ports.
Deliberately named a npm script "start:regtest:secondary" to indicate that this is intended for regtest use as the port is hardcoded to 29080 (the secondary regtest container). Feels like a major improvement - thanks again for the superb feedback! What do you think?

Also, with the docker image merged and built, one can use npm run regtest:rebuild with npm run start:regtest:secondary to try out the log feature! No more complicated steps as described in the original "How to test" description.

Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

Introduced "backend types" native and jam-standalone in the setupProxy script.

I like it! Way less confusing now.

tACK, Looks good to me ✅

@theborakompanioni theborakompanioni merged commit ace3734 into master Sep 9, 2022
@theborakompanioni theborakompanioni deleted the log-file branch September 9, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concept Wild idea, or too many details unknown yet enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: ability to retrieve logs
2 participants