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

[sozo] add trace logs #1867

Merged
merged 28 commits into from
May 1, 2024
Merged

[sozo] add trace logs #1867

merged 28 commits into from
May 1, 2024

Conversation

btirth
Copy link
Contributor

@btirth btirth commented Apr 22, 2024

Added trace logs for Sozo.
Closes #1409
Closes DOJ-160

Copy link
Contributor

@tarrencev tarrencev left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Great work @btirth!

As @tarrencev mentioned, we should use structured logging everywhere.

You have the good convention with the LOG_TARGET, now you'll have to switch to the correct format where the text should be a sentence with a capital letter and punctuation.
And for the variables, you have to assign them depending on what should be printed.

If you have any question, please let's know. You can find several examples of this structured logging into the code base already.

bin/sozo/src/args.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/fee.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/fee.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/migrate.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/events.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/clean.rs Outdated Show resolved Hide resolved
@btirth btirth requested review from glihm and tarrencev April 23, 2024 06:13
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

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

Project coverage is 70.13%. Comparing base (6427cdc) to head (a741fa9).

Files Patch % Lines
bin/sozo/src/commands/auth.rs 0.00% 20 Missing ⚠️
bin/sozo/src/commands/options/fee.rs 0.00% 17 Missing ⚠️
bin/sozo/src/commands/account.rs 13.33% 13 Missing ⚠️
bin/sozo/src/commands/events.rs 0.00% 12 Missing ⚠️
bin/sozo/src/commands/dev.rs 0.00% 10 Missing ⚠️
bin/sozo/src/commands/migrate.rs 0.00% 9 Missing ⚠️
bin/sozo/src/commands/execute.rs 0.00% 6 Missing ⚠️
bin/sozo/src/commands/options/transaction.rs 16.66% 5 Missing ⚠️
bin/sozo/src/commands/build.rs 42.85% 4 Missing ⚠️
bin/sozo/src/commands/options/account.rs 42.85% 4 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1867      +/-   ##
==========================================
- Coverage   70.28%   70.13%   -0.15%     
==========================================
  Files         315      315              
  Lines       35864    35993     +129     
==========================================
+ Hits        25206    25244      +38     
- Misses      10658    10749      +91     

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

@glihm glihm changed the title #1409 sozo trace logs [sozo] add trace logs Apr 23, 2024
Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Thanks for the work here!
Some other comments.

Also, it seems that your editor is always removing empty line at the end of the files. It's preferable to have this new line at the end of the file.

bin/sozo/src/args.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/register.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/register.rs Outdated Show resolved Hide resolved
bin/sozo/src/main.rs Outdated Show resolved Hide resolved
bin/sozo/src/main.rs Outdated Show resolved Hide resolved
bin/sozo/src/main.rs Outdated Show resolved Hide resolved
@btirth btirth requested a review from glihm April 24, 2024 03:19
Copy link
Member

@kariy kariy left a comment

Choose a reason for hiding this comment

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

i dont think we have to insert log for every stuff we're doing. it's only gonna introduce noise than actually useful info. i have sugggested some places where the logs can be removed.

besides, individual event logs arent particularly useful, i think its better to use spans wherever make sense instead (eg on the commands' main execution point)

wdyt @tarrencev @glihm ?

bin/sozo/src/args.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/clean.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/clean.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/build.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/build.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/init.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/init.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/mod.rs Outdated Show resolved Hide resolved
bin/sozo/src/main.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/register.rs Outdated Show resolved Hide resolved
@btirth btirth requested a review from kariy April 25, 2024 04:08
Copy link
Collaborator

@lambda-0x lambda-0x left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

i have few suggestion:

  • i dont think we need to manually specify LOG_TARGET by default the package name would be used which i think is good
  • for things like signer, world_address, which are static we should print them each in a single line (and preferrably during the beginning of execution) so they are easier to find.

@btirth
Copy link
Contributor Author

btirth commented Apr 26, 2024

@lambda-0x Thank you for your suggestions.

  • I've take Katana logging as reference, if LOG_TARGET in Katana if for any specific reason then I'll remove it from Sozo.
  • If we print signer and world_address on single line, there'll be no trace message for that log, as it doesn't make sense to copy same message. If that's fine I can do that as well.

@lambda-0x
Copy link
Collaborator

lambda-0x commented Apr 26, 2024

I've take Katana logging as reference, if LOG_TARGET in Katana if for any specific reason then I'll remove it from Sozo.

I think katana benefits from having a katana::cli namespace since most of its logging comes from core part of katana, but @kariy would have more info on why we do it that way for katana.

for sozo its not very useful since since we would need to manage them whenever doing any kind of refactor.

If we print signer and world_address on single line, there'll be no trace message for that log, as it doesn't make sense to copy same message. If that's fine I can do that as well.

i meant we should have a trace for each one of them in the beginning of sozo execution since they are static and wont change once they are calcuated (for e.g. world address if user hasn't specified it we take it from `Scarb.toml).

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Could you please double check that fetched is only used when something is actually fetched from the network.

I've made some modifications in Events, but may have missed some.

bin/sozo/src/commands/options/world.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/world.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/migrate.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/events.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/events.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/events.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/events.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/call.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/build.rs Outdated Show resolved Hide resolved
@btirth btirth requested a review from glihm April 27, 2024 03:38
@btirth
Copy link
Contributor Author

btirth commented Apr 29, 2024

Are any changes required or can we merge this one?

@lambda-0x
Copy link
Collaborator

@btirth can you please take a look at CI failures, try running the command they run locally.

@btirth
Copy link
Contributor Author

btirth commented Apr 29, 2024

@lambda-0x: I've fixed CI failures.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Thanks for your patience and iteration @btirth!
I you could address the latest comments, will merge after that. Great work!

bin/sozo/src/commands/dev.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/register.rs Outdated Show resolved Hide resolved
bin/sozo/src/main.rs Outdated Show resolved Hide resolved
bin/sozo/src/main.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/options/account.rs Outdated Show resolved Hide resolved
bin/sozo/src/commands/execute.rs Show resolved Hide resolved
bin/sozo/src/commands/dev.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

@btirth thanks for the work here!

@glihm glihm merged commit 683620c into dojoengine:main May 1, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SOZO] Improve Sozo logging
5 participants