hyperledger-iroha / iroha-2-docs

https://docs.iroha.tech/
Apache License 2.0
6 stars 26 forks source link

Tracking Issue for Configuration Reference as per RFC #392

Open 0x009922 opened 1 year ago

0x009922 commented 1 year ago

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 commented 1 year ago

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:

[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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 (https://github.com/hyperledger/iroha/issues/3364), but it is very unlikely to happen before Iroha 2.0 release. Thus, I will remove them from the config reference.

mversic commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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