metrics settings

[finiteprods]

Adds a settings struct for [metrics.influxdb] settings

pub struct InfluxSettings {
    /// The URL where InfluxDB is running.
    ///
    /// # Examples
    ///
    /// **TOML**
    /// ```text
    /// [metrics.influxdb]
    /// url = "http://localhost:8086"
    /// ```
    ///
    /// **Environment variable**
    /// ```text
    /// XAYNET_METRICS__INFLUXDB__URL=http://localhost:8086
    /// ```
    pub url: String,

    /// The InfluxDB database name.
    ///
    /// # Examples
    ///
    /// **TOML**
    /// ```text
    /// [metrics.influxdb]
    /// db = "test"
    /// ```
    ///
    /// **Environment variable**
    /// ```text
    /// XAYNET_METRICS__INFLUXDB__DB=test
    /// ```
    pub db: String,
}

[codecov]

Codecov Report

Merging #493 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #493   +/-   ##
=======================================
  Coverage   51.56%   51.56%           
=======================================
  Files          63       63           
  Lines        3105     3105           
=======================================
  Hits         1601     1601           
  Misses       1504     1504           

Impacted Files	Coverage Δ
rust/src/settings.rs	5.71% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab20f52...048d7b9. Read the comment docs.

[little-dude]

I suspect that some settings will be specific to the metrics backend we're using. Maybe we have different settings for each backend:

[metrics.influxdb]
url = "http://influxdb:8086"
store_name = "metrics"

[finiteprods]

fair point, I think namespacing [metrics.influxdb] is indeed useful

[Robert-Steiner]

Looks good👍
Do you know if we can make these settings optional? I mean for the whole settings object we could use #[cfg(feature = "metrics")] but I think it is nicer if we can just make it optional.

If you like you can also add the fields username and password. Both have the type String. But here it is important that both fields are optional because influx allows communication without authentication.

[Robert-Steiner]

Do you know if it's easy to check if the value is a valid URL?

For example you can use the type std::net::SocketAddr for ip addresses.
When the configuration file is parsed, SocketAddr::FromStr is called and it will automatically fail if it is not a valid IP address. We used that behaviour for ApiSettings::bind_address. Maybe there is something similar for urls in the std lib.

However, if it requires an additional library, I'm not sure it's worth it.

[little-dude]

We can easily check whether a URL is valid, whether is contains a hostname, what the scheme is, etc. But we cannot check that the domain is valid in that it resolves to an IP address without making a network call, and I think we should refrain from doing network calls when parsing a config file, because they can hang.

[finiteprods]

one thing we can certainly do I think is #[validate(url)]. I'll add that now...

[Robert-Steiner]

Right, I just thought of checking that scheme is valid.
But maybe it's not that important for now (just an idea I had in mind).

[finiteprods]

Looks good👍
Do you know if we can make these settings optional? I mean for the whole settings object we could use #[cfg(feature = "metrics")] but I think it is nicer if we can just make it optional.

If you like you can also add the fields username and password. Both have the type String. But here it is important that both fields are optional because influx allows communication without authentication.

thanks for reviewing. would you be happy if we postpone the "optionality" of the settings to another PR? because I had in mind other settings (in PET, especially) which should also be optional, so it could all be tackled there.

[Robert-Steiner]

Looks good👍
Do you know if we can make these settings optional? I mean for the whole settings object we could use #[cfg(feature = "metrics")] but I think it is nicer if we can just make it optional.
If you like you can also add the fields username and password. Both have the type String. But here it is important that both fields are optional because influx allows communication without authentication.

thanks for reviewing. would you be happy if we postpone the "optionality" of the settings to another PR? because I had in mind other settings (in PET, especially) which should also be optional, so it could all be tackled there.

Sounds good to me 👍

[little-dude]

Do you know if we can make these settings optional? I mean for the whole settings object we could use #[cfg(feature = "metrics")] but I think it is nicer if we can just make it optional.

I think we shouldn't hide it completely, because that means the config wouldn't parse based on whether the feature is there or not. A more user-friendly approach imo would be to log a warning when there's a metrics section in the config, but the metrics feature is not enabled.

Edit: sorry as I pressed "comment" I realized I had mis-read your comment. You're actually proposing not to hide the settings behind that flag.