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

Logging middleware should not be always-applied and better move to separate crate #107

Closed
iyuq opened this issue Dec 7, 2018 · 1 comment · Fixed by #222
Closed

Logging middleware should not be always-applied and better move to separate crate #107

iyuq opened this issue Dec 7, 2018 · 1 comment · Fixed by #222
Labels
feature A feature that's ready for implementation
Milestone

Comments

@iyuq
Copy link

iyuq commented Dec 7, 2018

Logging to console significantly affect the QPS of the framework. With the logging middleware, the QPS of the simple hello-world example drops from

Running 10s test @ http://localhost:8000
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.06ms    1.69ms  32.84ms   92.69%
    Req/Sec     2.63k   472.73     4.72k    81.59%
  105226 requests in 10.10s, 13.05MB read
Requests/sec:  10416.80
Transfer/sec:      1.29MB

to

Running 10s test @ http://localhost:8000
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.93ms    2.38ms  33.97ms   88.50%
    Req/Sec     1.36k   287.61     2.21k    74.75%
  54088 requests in 10.02s, 6.71MB read
Requests/sec:   5399.07
Transfer/sec:    685.43KB

I think we should give user the choice to choose whether or not use logging middleware.
To solve this, I think we can offer the choice by add a new constructor for app.
like

pub fn new(data: Data) -> App<Data> {
        let app = App {
            data,
            router: Router::new(),
            default_handler: BoxedEndpoint::new(async || http::status::StatusCode::NOT_FOUND),
        };
        app
}

pub fn default(data: Data) -> App<Data> {
        let logger = RootLogger::new();
        let mut app = App {
            data,
            router: Router::new(),
            default_handler: BoxedEndpoint::new(async || http::status::StatusCode::NOT_FOUND),
        };

        // Add RootLogger as a default middleware
        app.middleware(logger);
        app
}

Furthermore, I think it's better to separate the Logging middleware from the core framework, for example, we could move the tide to the tide-core crate to tide workspace, add a tide-logging crate for tide crate. Like the tree graph below:

tide
|-tide-core
|--Cargo.toml
|-tide-logging
|--Cargo.toml
|-Cargo.toml
@bIgBV
Copy link
Contributor

bIgBV commented Dec 8, 2018

The current state of logging is just a starting point to integrate logging into tide, there's a lot of discussion regarding this over at #8 and nothing is concrete yet. I like the idea of separating logging out into it's own crate in the workspace though. Please add your thoughts over at the other discussion as well :)

@yoshuawuyts yoshuawuyts added the feature A feature that's ready for implementation label Jan 9, 2019
@prasannavl prasannavl mentioned this issue May 15, 2019
8 tasks
@bIgBV bIgBV added this to the Sprint 2 milestone May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A feature that's ready for implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants