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

Tracking Issue for Configuration Reference as per RFC #392

Open
5 of 13 tasks
0x009922 opened this issue Sep 6, 2023 · 7 comments
Open
5 of 13 tasks

Tracking Issue for Configuration Reference as per RFC #392

0x009922 opened this issue Sep 6, 2023 · 7 comments
Assignees
Labels
A-important High-impact, important change L-config Configuration, customization M-epic Issues/PRs that depend on other items (e.g. tracking issues)
Milestone

Comments

@0x009922
Copy link
Contributor

0x009922 commented Sep 6, 2023

Description

This issue is created to track the progress of creating a new Configuration Reference according to the Configuration Overhaul RFC.

Context

Checklist

Take this tasks/discussions into account:

An outdated related task:

@0x009922
Copy link
Contributor Author

0x009922 commented Sep 6, 2023

Here is a bunch of questions/proposals I have now.

Use human-readable durations

There are plenty of configuration parameters (usually ended with _ms suffix) which defines some duration in milliseconds. It would be much more convenient for users to specify them in a human-readable form, like:

  • 1 second
  • 2 minutes
  • 1 hour 5 minutes 3 seconds
  • 300 ms
[sumeragi]
block_time = "5 seconds" # instead of 5_000

We could implement our own parser, or use something like humantime crate

Note: users might always opt-out and specify a number of milliseconds:

block_time = 5_000

Use human-readable byte sizes

Same as for durations:

[torii]
max_transaction_size = "32kb"

[wsv.wasm_runtime]
max_memory = "600 MiB"

See: humansize - Rust

network, sumeragi.network, p2p_addr etc

There is iroha_config::network::Configuration, which takes [network] namespace in the config. It has only actor_channel_capacity field, and according to my IDE, it is not used anywhere. I guess it was eventually moved to sumeragi.actor_channel_capacity and was not cleared.

I would like to move torii.p2p_addr and sumeragi.actor_channel_capacity into network or sumeragi.network namespace.

[sumeragi.network] # or just [network]?
address = "localhost:1339"
actor_channel_capacity = 10

address or addr?

There is address in Peer Ids (iroha_data_model::peer::model::PeerId, used in sumeragi.trusted_peers):

[[sumeragi.trusted_peers]]
address = "..."

Should we rename it to addr or use address everywhere? I would choose address.

Split telemetry

Current fields are:

  1. name
  2. url
  3. min_retry_period
  4. max_retry_delay_exponent
  5. file

According to the code, it seems that fields 1..4 are for "substrate" telemetry, and field 5 (file) is for "dev", "future" telemetry. There is no any conflict/dependency between them.

I propose to introduce substrate and file_output (or something like that) namespaces:

[telemetry.substrate]
name = "iroha"
url = "ws://localhost:8080"
min_retry_period = "5 seconds"
max_retry_delay_exponent = 4

[telemetry.file_output]
file = "~/iroha-dev-telemetry"

@Mingela
Copy link

Mingela commented Sep 6, 2023

As for the time we could use pretty standard format like ISO 8601. Though I'm totally fine with that humanize if that's convenient and does not introduce any awkward confusions.

Agree with the byte size

Agree with the network

Agree with address

Agree with telemetry, though I'd suggest to reformat that to a configurable/pluggable telemetry,
I'd avoid

[telemetry.file_output]
enabled = true
file = "~/iroha-dev-telemetry"

or

[telemetry]
mode = "file_output"

[telemetry.file_output]
file = "~/iroha-dev-telemetry"

but we might consider addressing that. Alternatively the telemetry could be represented by a separate config file with other, more complicated structure.

@6r1d
Copy link
Contributor

6r1d commented Sep 7, 2023

Use human-readable durations
Use human-readable byte sizes

I can't agree more: that'll help users a lot.

I would like to move torii.p2p_addr and sumeragi.actor_channel_capacity into network or sumeragi.network namespace.

Just "network" sounds good to me.

address or addr?

address. People would copy and paste these configs, if those are not generated, and the parameters would be more readable.

I propose to introduce substrate and file_output (or something like that) namespaces

Sounds good

Though I'm totally fine with that humanize if that's convenient and does not introduce any awkward confusions.

In my opinion, it depends on the case that is handled. ISO is applicable if and when there are long intervals and parsing for some of them has the potential to be ambiguous, otherwise it may be more distracting.

@0x009922
Copy link
Contributor Author

We discussed *.actor-channel-capacity parameters with @Erigara. It turned out that they aren't actually used anywhere ATM, and were left in order to not overcomplicate DevOps' life for current deployments. There is a chance that these options will be returned in some future (hyperledger-iroha/iroha#3364), but it is very unlikely to happen before Iroha 2.0 release. Thus, I will remove them from the config reference.

@mversic
Copy link
Contributor

mversic commented Feb 5, 2024

Here is a bunch of questions/proposals I have now.

Use human-readable durations

[sumeragi]
block_time = "5 seconds" # instead of 5_000

We could implement our own parser, or use something like humantime crate

I'm very much against this. I think config should be simple. Just using a suffix on the field name is the simplest thing and something everybody will know how to use. Otherwise, the user has to know what the proper format is and we have to implement our own parser for every field, etc. In this case I find that block_time_ms is a solution that gives us all the benefits and is the simplest possible

Note: users might always opt-out and specify a number of milliseconds:

block_time = 5_000

This is definitely a no. There should be no opting out because it's ambiguous. Config fields should be unambiguous

Use human-readable byte sizes

Same as for durations:

[torii]
max_transaction_size = "32kb"

[wsv.wasm_runtime]
max_memory = "600 MiB"

again the same, I would rather like to see max_transaction_size_bytes/max_transaction_size_b/max_transaction_size_b

@mversic
Copy link
Contributor

mversic commented Feb 5, 2024

network, sumeragi.network, p2p_addr etc

There is iroha_config::network::Configuration, which takes [network] namespace in the config. It has only actor_channel_capacity field, and according to my IDE, it is not used anywhere. I guess it was eventually moved to sumeragi.actor_channel_capacity and was not cleared.

remove actor_channel_capacity. We don't use actors anymore

I would like to move torii.p2p_addr and sumeragi.actor_channel_capacity into network or sumeragi.network namespace.

[sumeragi.network] # or just [network]?
address = "localhost:1339"
actor_channel_capacity = 10

just network is preferable

address or addr?

There is address in Peer Ids (iroha_data_model::peer::model::PeerId, used in sumeragi.trusted_peers):

[[sumeragi.trusted_peers]]
address = "..."

Should we rename it to addr or use address everywhere? I would choose address.

let's go with address, but preferably we should use {public_key}@@{address} format

@mversic
Copy link
Contributor

mversic commented Feb 5, 2024

Split telemetry

Current fields are:

1. `name`

2. `url`

3. `min_retry_period`

4. `max_retry_delay_exponent`

5. `file`

According to the code, it seems that fields 1..4 are for "substrate" telemetry, and field 5 (file) is for "dev", "future" telemetry. There is no any conflict/dependency between them.

I propose to introduce substrate and file_output (or something like that) namespaces:

[telemetry.substrate]
name = "iroha"
url = "ws://localhost:8080"
min_retry_period = "5 seconds"
max_retry_delay_exponent = 4

[telemetry.file_output]
file = "~/iroha-dev-telemetry"

I don't see a point

@nxsaken nxsaken added M-epic Issues/PRs that depend on other items (e.g. tracking issues) T-cli CLI documentation L-config Configuration, customization and removed documentation T-cli CLI documentation labels May 20, 2024
@nxsaken nxsaken added this to the 2.0.0-rc.1.0 milestone May 24, 2024
@nxsaken nxsaken added the A-important High-impact, important change label May 24, 2024
@nxsaken nxsaken pinned this issue May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-important High-impact, important change L-config Configuration, customization M-epic Issues/PRs that depend on other items (e.g. tracking issues)
Projects
None yet
Development

No branches or pull requests

5 participants