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 config debug-ability #4456

Merged
merged 51 commits into from
May 17, 2024

Conversation

0x009922
Copy link
Contributor

@0x009922 0x009922 commented Apr 17, 2024

Description

This PR contains a major improvement of the work done in #4239. This is a big step towards better debug-ability of Iroha configuration system due to better errors. And yet, there are still many things to improve further!

Key points:

  • New ConfigReader API, which handles configuration sources in a dynamic manner rather than relying on serde completely, thus giving high flexibility in how to handle different cases. It is also built targeting Iroha domain specifically and doesn't have any extra stuff. Much less boilerplate.
  • Heavy use of error_stack crate, which gives higher control over errors reporting than eyre does.
  • --trace-config works! Uses log & stderrlog crates for it. Seems to be not a heavy solution for this. It is completely separate from tracing stack.

Further enhancements

There are still many things that might be improved.

  • Enhance use of attachments API provided by error_stack:
    • Use hooks to colour them conditionally
    • Attach hints and suggestions
    • Unify attachments further as specific structs, not just format!
  • Use spans to config files, and build reports showing the actual file contents
  • Fine-tune API of ConfigReader, use better naming and abstractions, perhaps.
  • Optimise internals of ConfigReader. There is lots of naive code and extra allocations.
  • Find a way to reduce extra context changes in error_stack. Sometimes it would be really cool to just "forward" underlying context without changing it, but currently it's how error_stack is designed. So, we have sometimes stacks like "Failed to load config -> Failed to load config -> failed to read file system -> ..."
  • Possibly, introduce a slim compile feature for Iroha, which will remove --trace-config support alongside with log & stderrlog dependencies, and more
  • Use a single multihash string for private keys, thus removing necessity to set private keys in env as two separate variables (e.g. PRIVATE_KEY_ALGORITHM + PRIVATE_KEY_PAYLOAD, and removing cratchy env_custom API from ConfigReader
  • Migrate to Axum, finally! Errors handling in Torii might be improved a lot.

Linked isues

Closes #3470, #4299, #4300

Gallery

Note: I've changed some bits after making these screenshots.

Some terminal screenshots Pasted image 20240417100106 Pasted image 20240417160528 Pasted image 20240417160759 Pasted image 20240417160900 Pasted image 20240417161142 Pasted image 20240417161212 Pasted image 20240417161247 Pasted image 20240417161611 Pasted image 20240417162020

TODO

  • Document internals of iroha_config_base
  • Fix iroha_client_cli compilation failure
  • Enabled failing test back: fails_with_no_trusted... in cli/src/lib.rs
  • Self-review

@0x009922 0x009922 added Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST UI Something about the interface labels Apr 17, 2024
@0x009922 0x009922 self-assigned this Apr 17, 2024
@github-actions github-actions bot added api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha labels Apr 17, 2024
Copy link

@BAStos525

data_model/src/block.rs Outdated Show resolved Hide resolved
@nxsaken nxsaken changed the base branch from iroha2-dev to main April 17, 2024 15:00
@0x009922
Copy link
Contributor Author

@DCNick3, I would appreciate if you can take a look at the procmacro I wrote here, in config/base/derive.

@0x009922 0x009922 force-pushed the config-reader-api branch 2 times, most recently from 6b96f4d to 5af02bf Compare April 19, 2024 01:49
@0x009922
Copy link
Contributor Author

Debugging CI: new reports are really helpful!

image

@DCNick3 DCNick3 self-assigned this Apr 19, 2024
@DCNick3
Copy link
Contributor

DCNick3 commented Apr 19, 2024

A bit concerned with the inclusion of source code location in the error trace (the at some/source/location.rs lines). I don't think it's useful for users and questionably useful for iroha developers. Maybe remove it/gate being some verbosity flag?

@0x009922
Copy link
Contributor Author

A bit concerned with the inclusion of source code location in the error trace (the at some/source/location.rs lines). I don't think it's useful for users and questionably useful for iroha developers. Maybe remove it/gate being some verbosity flag?

I am concerned too. There are also complete backtraces attached, which are displayed in dev environment. Anyway, we can disable Location's printing using the hooks. I used it in tests to capture snapshots. Will think about it.

config/base/derive/src/lib.rs Outdated Show resolved Hide resolved
config/base/src/read.rs Outdated Show resolved Hide resolved
config/base/src/read.rs Outdated Show resolved Hide resolved
config/base/src/read.rs Outdated Show resolved Hide resolved
config/base/src/toml.rs Outdated Show resolved Hide resolved
config/base/derive/src/lib.rs Outdated Show resolved Hide resolved
config/base/derive/src/lib.rs Show resolved Hide resolved
config/base/derive/src/lib.rs Outdated Show resolved Hide resolved
config/base/derive/src/lib.rs Outdated Show resolved Hide resolved
config/base/derive/src/lib.rs Outdated Show resolved Hide resolved
@0x009922 0x009922 force-pushed the config-reader-api branch from 9b613a6 to 1aa3bf6 Compare April 22, 2024 00:25
0x009922 added a commit to 0x009922/iroha that referenced this pull request Apr 22, 2024
@0x009922 0x009922 marked this pull request as ready for review April 22, 2024 00:45
0x009922 and others added 24 commits May 17, 2024 19:38
Signed-off-by: Dmitry Balashov <[email protected]>
Co-authored-by: ⭐️NINIKA⭐️ <[email protected]>
Signed-off-by: 0x009922 <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
@0x009922 0x009922 force-pushed the config-reader-api branch from ca08d9d to d3dbf2c Compare May 17, 2024 10:38
@0x009922 0x009922 merged commit 9d91c77 into hyperledger-iroha:main May 17, 2024
15 of 17 checks passed
@0x009922 0x009922 deleted the config-reader-api branch May 17, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST UI Something about the interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[suggestion] Enhance configuration parsing with precise field locations
4 participants