Feature/config and clean shutdown #52

Rom3dius · 2024-03-26T14:06:29Z

New Features

Adds globally accessible static config
Adds TOML config file
Adds clean shutdown
Adds optional logging file

Deprecated

Removes loading server port from .env (dotenvy)

Loading the server port from .env could be brought back, but I do not see much of a need. The program will automatically generate a config file and run on port 8080 if no config exists, otherwise it uses existing config.

EDIT

No TOML
No global config
Dotenv compatible

Rom3dius · 2024-03-26T14:15:36Z

Please excuse the formatting, I'll fix it and squash the commits.

Avi-D-coder · 2024-03-26T14:25:45Z

kairos-server/src/config.rs

+    pub fn read_config() -> Config {
+        let config_file = "kairos_config.toml";
+        if !std::path::Path::new(config_file).exists() {
+            let default_config = Config {
+                server: Server {
+                    address: "0.0.0.0".to_string(),
+                    port: 8080,
+                },
+                log: Log {
+                    level: Level::TRACE,
+                    log_file: None,
+                }
+            };
+            let toml_string = toml::to_string_pretty(&default_config).unwrap();
+            std::fs::write(config_file, toml_string).expect("Failed to write config file, file permissions might be incorrect.");
+        }
+        let config: Config = toml::from_str(
+            &std::fs::read_to_string(config_file).expect("Failed to read config file, file permissions might be incorrect.")
+        ).expect("Failed to parse config file");
+
+        config
+    }


In general apps should not create state unnecessarily.
This can be an issue when running in readonly filesystems.
ENVs are used to avoid this problem, they also play better with docker and nix.

Would env variable/dotenv overrides and not panicking if it can't write the config file be a good compromise?

No servers should not be creating state.
I'd be fine with reading a config file, but we should never write one, unless told to by the user.
And if we are reading a config you have to ask where, and then we get into xdg which is more for apps anyway.

ENVs are the industry standard for a reason.
dotenvy turns a set of ENVs into a config file in the local directory (just like kairos_config.toml).

Avi-D-coder · 2024-03-26T14:46:03Z

Supporting logging to a file directly instead of piping stderr/stdout is definitely something we could support. But it should not be the default, and we should not duplicate standard functionality we get from tracing's env_logger support.

Take a look at the options from https://docs.rs/env_logger/latest/env_logger/
Try this to replicate the functionality of this PR.

cd ./kairos-server
RUST_LOG=trace cargo run > kairos-server.log

Rom3dius · 2024-03-26T14:58:09Z

Supporting logging to a file directly instead of piping stderr/stdout is definitely something we could support. But it should not be the default, and we should not duplicate standard functionality we get from tracing's env_logger support.

Take a look at the options from https://docs.rs/env_logger/latest/env_logger/ Try this to replicate the functionality of this PR.
cd ./kairos-server
RUST_LOG=trace cargo run > kairos-server.log

It isn't the default, it's disabled by default. Also isn't the default functionality, it saves the file and line the tracing occured. I originally intended to use fern, its also a logging library like env_logger. Would that of interest?

Avi-D-coder · 2024-03-26T15:03:24Z

It isn't the default, it's disabled by default. Also isn't the default functionality, it saves the file and line the tracing occured. I originally intended to use fern, its also a logging library like env_logger. Would that of interest?

This we can fix.
Try adding RUST_LOG=trace to the .env.

As for fern, IDK I have never used it, what does it provide better than tracing.
It seems more like a log alternative than a tracing/slog alternative.

Rom3dius · 2024-03-26T18:05:28Z

Okay, it now doesn't generate a kairos_config.toml file nor does it have defaults.
It loads configuration hierarchically:

dotenv / environment variables
kairos_config.toml

I added a small README.md that details configuring the server.

Added defaults to the .env file.

I'm still using tracing-subscriber, adding another crate for configuring logging makes little sense, they all use the same backend anyway. Also tracing-subscriber has an ecosystem around it supported by tokio, has drop in libraries for providing OpenTelemetry/Prometheus metrics.

removed log file/stdout config, initialize logger in config added optional log file with more detailed logging added back dotenv support, hierarchical config loading updated Cargo.lock

Avi-D-coder · 2024-03-26T18:30:23Z

I like the readme, the defaults, and the shutdown.
I still don't think adding all this other stuff to partially duplicate existing functionality is worth it.

I would strongly suggest just using .env as your config file.
The custom log level functionality is very limited compared to env_logger.

In the current state I am not likely to approve this PR.
If other people think an additional config option is worth it then I won't block merging, once error handling is improved.

Some general notes:
lazy_static is not needed, avoid globals when possible, config should be passed as a parameter for testability.
Errors need to be propagated up the call stack.
Use less restrictive crate versions.

Rom3dius · 2024-03-26T20:53:58Z

I like the readme, the defaults, and the shutdown. I still don't think adding all this other stuff to partially duplicate existing functionality is worth it.

I would strongly suggest just using .env as your config file. The custom log level functionality is very limited compared to env_logger.

In the current state I am not likely to approve this PR. If other people think an additional config option is worth it then I won't block merging, once error handling is improved.

Some general notes: lazy_static is not needed, avoid globals when possible, config should be passed as a parameter for testability. Errors need to be propagated up the call stack. Use less restrictive crate versions.

Global configuration has become such a common practice that tokio has an in-house lazy_static! they added within the last two weeks, I'll swap to it

because of the hierarchical loading of the config files, you can have a separate config for development and production, with production essentially just overriding the defaults provided by development

.env cannot represent complex structures, depending on business requirements that might be of interest, toml supports more types. it will also start to look very messy the more that has to be added to it. dotenv doesnt have the same seperation of concern toml can provide.

the custom log level functionality isn't hard to extend, tracing-subscriber supports everything env_logger does with it's filters.
again, seems pointless to use env_logger when tracing-subscriber supports everything env_logger does, plus it has drop in crates for Opentelemetry/Prometheus metrics, which will give us a lot of insights into performance and visibility of the application
https://tokio.rs/tokio/topics/tracing-next-steps

Avi-D-coder · 2024-03-26T21:22:48Z

Global configuration has become such a common practice that tokio has an in-house lazy_static! they added within the last two weeks, I'll swap to it

The use of lazy_static is not the issue, while there are more modern specialized replacements like the various flavors of OnceCell, I would not take issue with using the older lazy_static.

The problem is global variables are not the right choice for config.
A global can only be set once per process.
An app config needs to be passed in as a parameters such that you can test the server with multiple configs without duplicating all your logic in every integration test.

In general it's best to avoid implicit logical dependencies. This is why globals are discouraged across languages.

the custom log level functionality isn't hard to extend, tracing-subscriber supports everything env_logger does with it's filters.

env_logger/env-filter is just a standard way of configuring logging with the RUST_LOG ENV. It's a feature built into tracing-subscriber. It does not prevent the usage of opentelemetry or any other tracing-subscribers.

The point of using RUST_LOG is you don't need custom code for configuring tracing-subscriber in common ways. You still can extend it by using the API directly but the builtin env-filter covers common usage.

The current toml does not allow anything the existing code does not.
RUST_LOG covers a superset of the config options in the toml.

jonas089 · 2024-03-26T22:20:58Z

I think that a global config as an option (not default) can be useful. When deploying the casper stuff to cctl I would prefer to use a config file over .env.

Having .env as the default and an optional config file seems comprehensive and allows for more flexibility.

What I like about Rome's solution is that when using the casper client as a library to interact with the L1 I can access the correctly typed values without having to read from the environment.

Avi-D-coder · 2024-03-26T22:48:51Z

access the correctly typed values without having to read from the environment.

Environment variables should be parsed at server startup. This is common between both approaches.

Parts of config that you access in a handler should be passed as a readonly part of axum state not in a global variable or read from environment.

https://docs.rs/axum/latest/axum/extract/struct.State.html

jonas089 · 2024-03-27T09:48:23Z

But you can construct "State" from .env or a config file, there is no reason not to have an optional config file.

Rom3dius · 2024-03-27T11:41:38Z

Just because axum provides their axum::extract::State pattern doesn't mean we have to use it. Global references are useful because we don't need to propagate configuration or connections down from method to method. There's more to this program than just axum's handlers. It seems silly to have to add this State(state): State<AppState>, to each handler when you could instead do crate::config or crate::AppState and put it wherever you need it. Makes accessing the config the same regardless of where you are in the program, axum handler, background tasks, error handling etc.

marijanp

Not sure how this improves setting up our server. I personally prefer configuring through CLI arguments, especially if it's only a port and an address.
TOML as a configuration file format is questionable too for me.

marijanp · 2024-03-27T14:30:59Z

kairos-server/src/config.rs

-pub struct ServerConfig {
-    pub port: u16,
-}
+fn serialize_level_filter<S>(level: &Level, serializer: S) -> Result<S::Ok, S::Error>


there is a crate called tracing-serde https://github.com/tokio-rs/tracing/tree/master/tracing-serde

Thank you! Can't believe I missed this.

Turns out this is for serializing logs to a machine readable format. Not useful for serializing/deserialzing the log level.

marijanp · 2024-03-27T14:31:47Z

kairos-server/src/config.rs

+
+#[derive(Debug, Clone, Deserialize, Serialize)]
+pub struct Server {
+    pub address: String,


I think this should be a URL

I guess yeah, do we have a crate with a Uri/Url type included? Are we planning on using the http crate?

What is the type that axum expects?

SocketAddr, the Config struct has an impl that creates it

marijanp · 2024-03-27T14:33:16Z

kairos-server/src/config.rs

+}
+
+#[allow(clippy::new_without_default)]
+impl Settings {


Isn't there a crate that takes a type and just derives the toml serializer/ parser

This allow clippy statement is because Avi told me not to construct state. There were default parameters in place before and the allow clippy wasn't needed. If I put the default function back, then the config crate will first construct the default, then load from toml, then dotenv, and then environment. ATM it will simply panic if there are parameters it can't gather from all those sources.

marijanp · 2024-03-27T14:35:13Z

kairos-server/src/main.rs

-
-    let config = ServerConfig::from_env()
-        .unwrap_or_else(|e| panic!("Failed to parse server config from environment: {}", e));
+    CONFIG.initialize_logger();


Semantically it is not the config that should initialize logging. It's the server that should initialize logging given a configuration

I don't mind moving the logging configuration to main.rs or out of config.rs. Do you have a suggestion?

marijanp · 2024-03-27T14:36:58Z

kairos-server/src/main.rs

+            .expect("Failed to install Ctrl+C handler");
+    };
+
+    let terminate = async {


I'm pretty sure this is what tokio already does for us when we send SIGTERM. There is no need to implement this ourselves, especially given that we don't need to handle any special conditions when we terminate

Different behavior is observable. Will probably be useful to wait for database connections to close before shutdown in the future as well.

The crate I'm using for loading the config is super flexible, I could add CLI options as well if that's something you'd like to see. I would really like to leave the toml config in place however, because it's flexible. For example, if I or anyone else wants to provide a list of nodes for the deposit listener to connect to, or more likely a list of nodes we know are trustworthy or low latency, toml is pretty practical. I can go to the config file and simply type the following:

[[nodes]] address = "192.168.1.1" port = 11101 [[nodes]] address = "12.168.1.1" port = 11101

But dotenv out of the box doesn't support complex structures and while a cli could definitely do it, it's a little cumbersome.

No one is being forced to use the toml file, it's simply an option and only costs resources on startup not during runtime. Everyone can stick with dotenv and potentially CLI options if we want to go down that route.

The issue is it's duplicate code that does not currently add functionality.
In the future it might make sense, but I don't anticipate having a complex set of args.

In this case nodes can be a CSV String address:port,*.
If we have a super complex set of structured args in the future we can revisit using toml.

koxu1996 · 2024-03-30T10:32:56Z

Here are my thoughts on each feature:

👍 Clean Shutdown

It will probably be useful in the future to wait for database connections to close before shutdown as well.

I am 100% sure we will soon need to handle the shutdown of multiple components, not only the database when the DA layer is built, but also the Casper event state synchronizer upon its implementation and integration.

Currently, the main binary is kairos-server, but I think we will switch to kairos-node, which will run the server (API) as part of the supervision tree. This transition requires a graceful shutdown.

👍 Optional Logging File

This feature is really useful for infrastructure operators. Instead of figuring out how to redirect stdout - which might vary depending on whether you run Kairos with systemd, nohup, Docker, etc. - it can simply be enabled in the config.

👍 TOML Config File

I understand the concerns of others that it's just another representation of the config, offering no immediate benefit with the added code. However, I can envision TOML's support for arrays becoming handy soon.

Another reason to use TOML is the improved semantics - I dislike seeing .env files where everything is just a string.

❓ Globally Accessible Static Config

This sounds like an anti-pattern; do we really need it?

@Rom3dius, except for the static global config, I have no other concerns 😉. For future reference, my recommendation is to avoid bundling multiple features in one PR; it will make it easier to avoid being blocked on individual parts of it.

I am convinced that TOML support is something we want. However, if @Avi-D-coder and @marijanp insist on avoiding it, perhaps you could extract that work into a separate enhancement-labeled PR. It could remain pending for some time until we have a more complex config that becomes cumbersome to manage with plain .env.

marijanp · 2024-04-01T14:33:36Z

Here are my thoughts on each feature:

👍 Clean Shutdown

It will probably be useful in the future to wait for database connections to close before shutdown as well.

I am 100% sure we will soon need to handle the shutdown of multiple components, not only the database when the DA layer is built, but also the Casper event state synchronizer upon its implementation and integration.

Currently, the main binary is kairos-server, but I think we will switch to kairos-node, which will run the server (API) as part of the supervision tree. This transition requires a graceful shutdown.

Can you explain why this special handling when the instance runs out of scope could not be part of the Drop trait, which normally gets exectured when the application shuts down.
This is also the case for the database connections: it would be very awkward if the datababase does not manage the connections itself.

👍 Optional Logging File

This feature is really useful for infrastructure operators. Instead of figuring out how to redirect stdout - which might vary depending on whether you run Kairos with systemd, nohup, Docker, etc. - it can simply be enabled in the config.

A service like this would run in a systemd service and systemd comes with systemd-journald which keeps track of logs and allows for querying with journalctl

From the manual:

systemd-journald is a system service that collects and stores logging data. It creates and maintains structured, indexed journals based on logging information that is received from a variety of sources:

👍 TOML Config File

I understand the concerns of others that it's just another representation of the config, offering no immediate benefit with the added code. However, I can envision TOML's support for arrays becoming handy soon.

Another reason to use TOML is the improved semantics - I dislike seeing .env files where everything is just a string.

Config files are awkward as I need to go ahead and create a file configuring things that actually should have good default values. And in this case I create a file to only configure an address and port.

The config file assumes a specific location that I don't know without reading the code. It could be in the CWD or in the xdg-config directory. Or sometimes I want it somewhere else. How do I configure this?

Thus both approaches environment variables or config files are not very explicit when I call the app. Instead what I like is a CLI with good defaults, because when I run kairos-server --help I can get all the information I need to run it.

Rom3dius · 2024-04-01T20:39:45Z

Can you explain why this special handling when the instance runs out of scope could not be part of the Drop trait, which normally gets exectured when the application shuts down. This is also the case for the database connections: it would be very awkward if the datababase does not manage the connections itself.

It's tricky to implement a drop trait for futures, you need to call task::spawn and wrap data in an Arc before passing it in so it doesn't go out of scope, otherwise you can't spawn any async code.

A service like this would run in a systemd service and systemd comes with systemd-journald which keeps track of logs and allows for querying with journalctl

From the manual:

systemd-journald is a system service that collects and stores logging data. It creates and maintains structured, indexed journals based on logging information that is received from a variety of sources:

A service like this could also be containerized, docker doesn't log to systemd by default. Depending on what tools an operator is using for insight, or how the operator has their infrastructure set up, they could collect that log file automatically and store it should anything go wrong. tracing-subscriber also allows for different logging outputs to be configured differently, so we can include more (or less) information in the log file and leave stdout for cleaner more human-readable logs. Potential use-case would be the following, log only warnings or worse to the log file, ingest using a loki ingester, on Grafana send webhooks to slack/discord/zulip warning the operator that their might be an issue with the application.

Config files are awkward as I need to go ahead and create a file configuring things that actually should have good default values. And in this case I create a file to only configure an address and port.

I agree having to manually create a config file and populate it is awkward, in the original pull request upon startup if the app didn't discover a config file it generated a default file with sensible defaults. A CLI for a service is a bit anti-pattern, this is reflected by the fact that all of the configuration crates I looked at on crates.io do not come with CLI parsing. What could also be useful is implementing the following: if a path is provided as an argument, create/load the config from there.

The config file assumes a specific location that I don't know without reading the code. It could be in the CWD or in the xdg-config directory. Or sometimes I want it somewhere else. How do I configure this?

Therefore I think there should be a config file generated automatically if one is missing and that one can specified as a command line argument. I disagree that any assumptions have to be made when using the config file instead of a CLI, because the information pertaining to the usage of the config is already in the README, as is standard practice for such a service.

koxu1996 · 2024-04-02T09:56:40Z

@marijanp

Can you explain why this special handling when the instance runs out of scope could not be part of the Drop trait, which normally gets exectured when the application shuts down.

Coordinating shutdown goes beyond what the Drop trait can handle. The process usually begins by sending a stop signal to components and waiting until they finish processing any tasks that have already started. In many cases, a specific order of component shutdown is required - for example, stopping the processing of requests before disconnecting from the database. This level of control cannot be achieved with just the Drop trait.

A service like this would run in a systemd service and systemd comes with systemd-journald which keeps track of logs and allows for querying with journalctl

This assumption applies to our initial production infrastructure only. Consider other use cases where Nix with systemd is not utilized. We can make life easier for developers who might want to run it differently.

Config files are awkward as I need to go ahead and create a file configuring things that actually should have good default values. And in this case I create a file to only configure an address and port.

The config file assumes a specific location that I don't know without reading the code. It could be in the CWD or in the xdg-config directory. Or sometimes I want it somewhere else. How do I configure this?

Thus both approaches environment variables or config files are not very explicit when I call the app. Instead what I like is a CLI with good defaults, because when I run kairos-server --help I can get all the information I need to run it.

If you're expressing concerns about the nature of config files, it's worth noting that a .env file is essentially a config file too. Its location is typically assumed to be in the current working directory (CWD) or a parent directory, but this isn't documented when running kairos-server via the CLI - a point that certainly warrants improvement in future updates. However, I haven't noted any specific objections to using TOML format.

Avi-D-coder · 2024-04-02T14:21:24Z

This assumption applies to our initial production infrastructure only. Consider other use cases where Nix with systemd is not utilized. We can make life easier for developers who might want to run it differently.

Docker apps are also easier to configure with stateless envs or cli.

Getting logs out of a file from docker or nix is not exactly fun, we should use a different networked tracing subscriber or stdout.

https://docs.docker.com/reference/cli/docker/container/logs/

marijanp · 2024-04-02T17:59:03Z

Can you explain why this special handling when the instance runs out of scope could not be part of the Drop trait, which normally gets exectured when the application shuts down. This is also the case for the database connections: it would be very awkward if the datababase does not manage the connections itself.

It's tricky to implement a drop trait for futures, you need to call task::spawn and wrap data in an Arc before passing it in so it doesn't go out of scope, otherwise you can't spawn any async code.

I don't understand the issue here a Future is a trait, as Drop is. If your structs takes care of cleaning up it's resources which it should do by implementing the drop trait. Drop will be called by rust.
https://rust-book.cs.brown.edu/ch15-03-drop.html#running-code-on-cleanup-with-the-drop-trait

Also I still can't see any of the code you are referring to, that needs such a cleanup at this point

A service like this would run in a systemd service and systemd comes with systemd-journald which keeps track of logs and allows for querying with journalctl
From the manual:

systemd-journald is a system service that collects and stores logging data. It creates and maintains structured, indexed journals based on logging information that is received from a variety of sources:

A service like this could also be containerized, docker doesn't log to systemd by default. Depending on what tools an operator is using for insight, or how the operator has their infrastructure set up, they could collect that log file automatically and store it should anything go wrong. tracing-subscriber also allows for different logging outputs to be configured differently, so we can include more (or less) information in the log file and leave stdout for cleaner more human-readable logs. Potential use-case would be the following, log only warnings or worse to the log file, ingest using a loki ingester, on Grafana send webhooks to slack/discord/zulip warning the operator that their might be an issue with the application.

You can create docker containers directly from nixos system configurations.
Also docker containers come with a linux base usually, which comes with systemd.

Config files are awkward as I need to go ahead and create a file configuring things that actually should have good default values. And in this case I create a file to only configure an address and port.

I agree having to manually create a config file and populate it is awkward, in the original pull request upon startup if the app didn't discover a config file it generated a default file with sensible defaults. A CLI for a service is a bit anti-pattern, this is reflected by the fact that all of the configuration crates I looked at on crates.io do not come with CLI parsing. What could also be useful is implementing the following: if a path is provided as an argument, create/load the config from there.

Can you elaborate why a CLI is an antipattern? Just because everybody else is not providing a CLI in crates.io, it I'm not convinced.

I'm ok with adding logging to a file through an option, as long as it's not the default

The config file assumes a specific location that I don't know without reading the code. It could be in the CWD or in the xdg-config directory. Or sometimes I want it somewhere else. How do I configure this?

Therefore I think there should be a config file generated automatically if one is missing and that one can specified as a command line argument. I disagree that any assumptions have to be made when using the config file instead of a CLI, because the information pertaining to the usage of the config is already in the README, as is standard practice for such a service.
I'm saying that a manual config creation for just an address and a port is an overload. State should not be generated by the application as @Avi-D-coder pointed out.

Rom3dius · 2024-04-03T12:30:22Z

Can you explain why this special handling when the instance runs out of scope could not be part of the Drop trait, which normally gets exectured when the application shuts down. This is also the case for the database connections: it would be very awkward if the datababase does not manage the connections itself.

I don't understand the issue here a Future is a trait, as Drop is. If your structs takes care of cleaning up it's resources which it should do by implementing the drop trait. Drop will be called by rust. https://rust-book.cs.brown.edu/ch15-03-drop.html#running-code-on-cleanup-with-the-drop-trait

It's tricky to implement a drop trait for futures, you need to call task::spawn and wrap data in an Arc before passing it in so it doesn't go out of scope, otherwise you can't spawn any async code.
https://rust-lang.github.io/async-fundamentals-initiative/roadmap/async_drop.html
https://www.reddit.com/r/rust/comments/15ilsl4/announcing_asyncdropper_the_least_worst_asyncdrop/
https://www.qovery.com/blog/common-mistakes-with-rust-async/
There's a lot of different issues implementing async Drop.

Also I still can't see any of the code you are referring to, that needs such a cleanup at this point

The axum API already uses this code to clean up. It stops accepting new requests and finishes serving existing ones instead of simply interrupting them.

You can create docker containers directly from nixos system configurations. Also docker containers come with a linux base usually, which comes with systemd.

The host running docker will not log to systemd without additional configuration.

Can you elaborate why a CLI is an antipattern? Just because everybody else is not providing a CLI in crates.io, it I'm not convinced.

I'm not completely opposed to CLI arguments, even if I don't think they have a place in a service.

I'm ok with adding logging to a file through an option, as long as it's not the default

It's still not the default.

Therefore I think there should be a config file generated automatically if one is missing and that one can specified as a command line argument. I disagree that any assumptions have to be made when using the config file instead of a CLI, because the information pertaining to the usage of the config is already in the README, as is standard practice for such a service.
I'm saying that a manual config creation for just an address and a port is an overload. State should not be generated by the application as @Avi-D-coder pointed out.

Except it's already not just an address and port, its also log level and optional log file. Then later it will be another port for prometheus metrics, then there's the verifier contract address, the path of the secret key and whatever else I couldn't think of off the top of my head. This is how the config ended up looking in the bare-minimum prototype Jonas scraped together.

address = "0.0.0.0"
port = 8080

[db]
address = "0.0.0.0"
port = 5432
username = "kairos"
password = "changeme"
database = "kairos"

[node]
address = "127.0.0.1"
port = 11101
counter_uref = "uref-"
dict_uref = "uref-"
secret_key_path = "/"
chain_name = "cspr-dev-cctl"
verifier_contract = "contract-"

[log]
level = "info"
file_output = "kairos.log"
stdout = true

fixed kairos-config.toml in gitignore

Avi-D-coder · 2024-04-03T15:06:30Z

It's tricky to implement a drop trait for futures, you need to call task::spawn and wrap data in an Arc before passing it in so it doesn't go out of scope, otherwise you can't spawn any async code.
https://rust-lang.github.io/async-fundamentals-initiative/roadmap/async_drop.html
https://www.reddit.com/r/rust/comments/15ilsl4/announcing_asyncdropper_the_least_worst_asyncdrop/
https://www.qovery.com/blog/common-mistakes-with-rust-async/
There's a lot of different issues implementing async Drop.

To clarify here, the async Drop issue is not about if a future implements Drop.
The idea of AsyncDrop is that calling async code in a destructor is useful.
This cannot currently be done nicely, you have to manually hook into the async runtime, we may get a solution to this with effects in the future. This is not an issue for us here.

The contents owned by a future *will (men::forget) have drop called just like in any other context.

I do think shutdown code is useful.
I have to add a shutdown for my trie state thread in the future anyway.
If you can break the shutdown code into a new PR, that'd be great.

koxu1996 · 2024-04-03T22:14:26Z

This assumption applies to our initial production infrastructure only. Consider other use cases where Nix with systemd is not utilized. We can make life easier for developers who might want to run it differently.

Docker apps are also easier to configure with stateless envs or cli.

Getting logs out of a file from docker or nix is not exactly fun, we should use a different networked tracing subscriber or stdout.

https://docs.docker.com/reference/cli/docker/container/logs/

@Avi-D-coder Adding --log-file option does not break statelessness, as long as /dev/stdout is the default value. It could behave like curl command.

Avi-D-coder · 2024-04-03T22:42:47Z

It's the writing of the log file in the container that I don't like, that's what stdout and
various other tracing layers are for.

The state I don't like is writing a default config if the config is not found.

I consider these issues separate since we could add an option to log to a file using ENVs, CLI, or a Config file. We just would not use it in prod or testing.
And when developing writing logs to a file is just a redirect > kairos.log.
I don't see a value in adding code that re-implements this.
However I'd be fine with adding it as a self contained small PR.

marijanp · 2024-05-15T18:16:13Z

Clean shutdown was merged in #93

Avi-D-coder reviewed Mar 26, 2024

View reviewed changes

Rom3dius force-pushed the feature/config-and-clean-shutdown branch from 65d37ef to 791f8be Compare March 26, 2024 17:55

Rom3dius force-pushed the feature/config-and-clean-shutdown branch from 791f8be to aa3d1e8 Compare March 26, 2024 18:25

added toml config and clean shutdown for axum

da2ad95

removed log file/stdout config, initialize logger in config added optional log file with more detailed logging added back dotenv support, hierarchical config loading updated Cargo.lock

Rom3dius force-pushed the feature/config-and-clean-shutdown branch from aa3d1e8 to da2ad95 Compare March 26, 2024 18:28

marijanp reviewed Mar 27, 2024

View reviewed changes

marijanp assigned Rom3dius Mar 28, 2024

Rom3dius added 3 commits March 30, 2024 14:07

removed global config, removed toml

0a233f0

Merge branch 'main' into feature/config-and-clean-shutdown

011036b

fix fmt + clippy

df02b60

added toml back, updated README

ad79709

fixed kairos-config.toml in gitignore

marijanp closed this May 15, 2024

marijanp deleted the feature/config-and-clean-shutdown branch May 30, 2024 15:04

Feature/config and clean shutdown #52

Feature/config and clean shutdown #52

Conversation

Rom3dius commented Mar 26, 2024 • edited Loading

New Features

Deprecated

EDIT

Rom3dius commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avi-D-coder commented Mar 26, 2024

Rom3dius commented Mar 26, 2024

Avi-D-coder commented Mar 26, 2024

Rom3dius commented Mar 26, 2024

Avi-D-coder commented Mar 26, 2024 • edited Loading

Rom3dius commented Mar 26, 2024 • edited Loading

Avi-D-coder commented Mar 26, 2024

jonas089 commented Mar 26, 2024 • edited Loading

Avi-D-coder commented Mar 26, 2024

jonas089 commented Mar 27, 2024

Rom3dius commented Mar 27, 2024

marijanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rom3dius Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koxu1996 commented Mar 30, 2024

👍 Clean Shutdown

👍 Optional Logging File

👍 TOML Config File

❓ Globally Accessible Static Config

marijanp commented Apr 1, 2024 • edited Loading

👍 Clean Shutdown

👍 Optional Logging File

👍 TOML Config File

Rom3dius commented Apr 1, 2024 • edited Loading

koxu1996 commented Apr 2, 2024 • edited Loading

Avi-D-coder commented Apr 2, 2024

marijanp commented Apr 2, 2024

Rom3dius commented Apr 3, 2024 • edited Loading

Avi-D-coder commented Apr 3, 2024

koxu1996 commented Apr 3, 2024

Avi-D-coder commented Apr 3, 2024

marijanp commented May 15, 2024

Rom3dius commented Mar 26, 2024 •

edited

Loading

Avi-D-coder commented Mar 26, 2024 •

edited

Loading

Rom3dius commented Mar 26, 2024 •

edited

Loading

jonas089 commented Mar 26, 2024 •

edited

Loading

Rom3dius Mar 27, 2024 •

edited

Loading

marijanp commented Apr 1, 2024 •

edited

Loading

Rom3dius commented Apr 1, 2024 •

edited

Loading

koxu1996 commented Apr 2, 2024 •

edited

Loading

Rom3dius commented Apr 3, 2024 •

edited

Loading