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

Add Zap support for Fiber. #20

Merged
merged 8 commits into from
Oct 14, 2021
Merged

Add Zap support for Fiber. #20

merged 8 commits into from
Oct 14, 2021

Conversation

efectn
Copy link
Member

@efectn efectn commented Oct 9, 2021

Faster than Fiber's offical logger.

Offical:

Running 20s test @ http://localhost:3000/
  2 threads and 128 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     3.30ms    3.56ms  24.78ms   85.56%
    Req/Sec    23.18k     1.75k   27.35k    70.00%
  923992 requests in 20.05s, 114.55MB read
Requests/sec:  46078.06
Transfer/sec:      5.71MB

Fiberzap:

Running 20s test @ http://localhost:3000/
  2 threads and 128 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.27ms    1.01ms  11.84ms   76.41%
    Req/Sec    41.93k     3.59k   50.16k    78.25%
  1671296 requests in 20.06s, 207.20MB read
Requests/sec:  83330.87
Transfer/sec:     10.33MB

But i need to decrease allocs/op, B/op and ns/op before the merging it. I need your reviews to fix it :)


{"level":"info","msg":"Success","latency":"103ns","status":200,"method":"GET","url":"/"}
{"level":"info","msg":"Success","latency":"101ns","status":200,"method":"GET","url":"/"}
{"level":"info","msg":"Success","latency":"94ns","status":200,"method":"GET","url":"/"}
  331002	      3731 ns/op	     261 B/op	       2 allocs/op
PASS

Copy link
Contributor

@Jictyvoo Jictyvoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put some comments.I don't know very much about this logger, sorry if I'm saying something wrong

fiberzap/config.go Outdated Show resolved Hide resolved
fiberzap/config.go Outdated Show resolved Hide resolved
fiberzap/config.go Outdated Show resolved Hide resolved
fiberzap/zap.go Show resolved Hide resolved
fiberzap/zap.go Outdated Show resolved Hide resolved
@ReneWerner87
Copy link
Member

@Jictyvoo can you check the stuff again -> when you approve it, i will merge it

@efectn
Copy link
Member Author

efectn commented Oct 13, 2021

@Jictyvoo can you check the stuff again -> when you approve it, i will merge it

I need to recheck benchmarks before merging it. Can you wait a bit?

@ReneWerner87
Copy link
Member

ReneWerner87 commented Oct 13, 2021

sure

fiberzap/zap.go Outdated Show resolved Hide resolved
@efectn
Copy link
Member Author

efectn commented Oct 14, 2021

@ReneWerner87 @Jictyvoo I think everything is OK.

@ReneWerner87 ReneWerner87 merged commit 637074c into gofiber:main Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants