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: improve gnodev logging #1790

Merged
merged 24 commits into from
Apr 14, 2024
Merged

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Mar 18, 2024

This PR enhances the gnodev logger for easier debugging:

  • Use charm log for better logging and improved display
  • Include a way to manage genesis delivery transactions result during initialisation using a custom handler in gnoland. This is currently the only way I've found to manually handle errors. Refer to this commit for more details: ecd1109
  • Enhance column display
  • Stack can be available if --verbose is specified

Screenshot 2024-03-18 at 16 36 33

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Mar 18, 2024
@gfanton gfanton changed the title wip: improve gnodev error handler wip: improve gnodev logging Mar 18, 2024
Signed-off-by: gfanton <[email protected]>
This was referenced Mar 19, 2024
@gfanton gfanton force-pushed the feat/gnodev-logger branch from 1f8a5df to 7e95f3f Compare March 19, 2024 12:23
Signed-off-by: gfanton <[email protected]>
@zivkovicmilos
Copy link
Member

@gfanton
What is the status of this PR?

@gfanton
Copy link
Member Author

gfanton commented Mar 29, 2024

@zivkovicmilos
I was waiting for feedback on the error display. I will make some slight adjustments, and then it will be ready for review.

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 16.00000% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 45.07%. Comparing base (2a9a8f5) to head (4a6baa4).
Report is 3 commits behind head on master.

Files Patch % Lines
gno.land/pkg/gnoland/app.go 0.00% 13 Missing ⚠️
gno.land/pkg/gnoland/node_inmemory.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1790      +/-   ##
==========================================
- Coverage   45.08%   45.07%   -0.01%     
==========================================
  Files         464      464              
  Lines       68014    68023       +9     
==========================================
- Hits        30661    30660       -1     
- Misses      34778    34786       +8     
- Partials     2575     2577       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gfanton added 3 commits March 29, 2024 15:45
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
@gfanton gfanton changed the title wip: improve gnodev logging feat: improve gnodev logging Mar 29, 2024
Signed-off-by: gfanton <[email protected]>
@gfanton gfanton force-pushed the feat/gnodev-logger branch from bb30dc8 to 4d9483e Compare March 31, 2024 09:58
@gfanton gfanton marked this pull request as ready for review April 2, 2024 07:13
@gfanton gfanton requested a review from moul as a code owner April 2, 2024 07:13
Signed-off-by: gfanton <[email protected]>
@leohhhn leohhhn requested a review from a team April 2, 2024 07:41
Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Hey, I played around with this a little. I recorded looms (sorry for two - got cut off):
Loom 1
Loom 2

Seems that genesis TX loading is not working properly and reloading the state is failing for some reason (ungracefully). See the looms, and lets discuss, not fully sure what's up.

EDIT:

OK, I think I figured out what the issue is. Monorepo I used to install this already has a p/demo/memeland & r/demo/memeland package & realm, and I was trying to load in the package with the same path. How can we handle this error better?

EDIT2: Yup, can confirm that the above was the issue. Can we handle this edge case? Check already deployed packages against the gno.mods you're trying to run with gnodev, and error out?

Also, the genesis TXs still aren't being loaded - seems like a separate issue.

@gfanton
Copy link
Member Author

gfanton commented Apr 8, 2024

@leohhhn Wow! Good catch, thanks! I believe Genesis TX FIle shouldn't load by default during development. It could be loaded with a separate flag, making it a feature rather than an issue 😛. As for the other bug, I'm unsure about the best approach. I'm concerned that overriding a package for the user might cause unexpected behavior. I suggest providing a clear error message for duplicate packages and a flag(?) for users to explicitly override the package.
In any case, could you open two issues for this? It's not related to this PR. The genesis I load in gnodev is a crafted one with all the added packages (not the file from the monorepo). What I did in this PR is add a way to manually handle errors when delivering transactions from the genesis. This allows me to properly handle and display errors in gnodev.

@zivkovicmilos
Copy link
Member

It could be loaded with a separate flag, making it a feature rather than an issue 😛

I'm glad you brought that up: #1883 😎

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Love this, now we have a reference implementation 💯

contribs/gnodev/pkg/dev/node.go Show resolved Hide resolved
Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Thank you for figuring out what the issue was with the panic when exiting or invalid syntax (it turned out to be that I was using Go 1.21 and for some reason it was failing).

Let's tackle other issues and possible improvements I mentioned after merging this 🙏

@leohhhn
Copy link
Contributor

leohhhn commented Apr 13, 2024

@gfanton is this good to merge?

@gfanton
Copy link
Member Author

gfanton commented Apr 14, 2024

@leohhhn I was waiting for potential additional reviews, but i guess we can merge it now

@gfanton gfanton merged commit 0cf65ee into gnolang:master Apr 14, 2024
184 checks passed
@gfanton gfanton deleted the feat/gnodev-logger branch April 14, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: No status
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants