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 req uuid in logger #425

Closed
wants to merge 2 commits into from
Closed

add req uuid in logger #425

wants to merge 2 commits into from

Conversation

cl1337
Copy link
Contributor

@cl1337 cl1337 commented Aug 6, 2018

▶ go test -bench=BenchmarkLogger -count=1 -benchmem ./runtime/...
goos: darwin
goarch: amd64
pkg: github.com/uber/zanzibar/runtime
BenchmarkLogger/per_endpoint,_light_log-8         	 2000000	       904 ns/op	     112 B/op	       2 allocs/op
BenchmarkLogger/per_endpoint,_medium_log-8        	   10000	    145110 ns/op	   81768 B/op	      80 allocs/op
BenchmarkLogger/per_endpoint,_heavy_log-8         	    3000	    542880 ns/op	  327410 B/op	     180 allocs/op
BenchmarkLogger/per_request,_light_log-8          	   20000	    137634 ns/op	  495693 B/op	       7 allocs/op
BenchmarkLogger/per_request,_medium_log-8         	   10000	   3358354 ns/op	16740449 B/op	       8 allocs/op
BenchmarkLogger/per_request,_heavy_log-8          	   10000	  10993113 ns/op	42676631 B/op	      15 allocs/op

@coveralls
Copy link

coveralls commented Aug 7, 2018

Coverage Status

Coverage decreased (-0.3%) to 62.143% when pulling be5e372 on cli/extend-context-logger into 840744e on master.

@ChuntaoLu
Copy link
Contributor

One important thing we need to keep in mind is that per request logger does not necessarily mean only one child logger per request, depending on the scope, many more child logger may be in need, e.g. a task group may create a child logger and each individual tasks might also have a scope for child logger, etc. So scale is nondeterministic.

@jacobgreenleaf and @srivastavankit pointed out the logging approach that Google App Engine uses, which we could leverage. https://cloud.google.com/appengine/docs/standard/go/logs/

@cl1337
Copy link
Contributor Author

cl1337 commented Aug 9, 2018

thanks @ChuntaoLu for the feedback

I have deployed this branch to a production service, it also proved having per request sub logger might not be a good idea for us

in benchmark we compared per req logging and per endpoint logging based on the number of log entries during a request's life cycle, and the payload size of the request:
for our production use case:

  • we focus more on error log entry
  • request usually terminated upon its first error
  • 99% case a request left <10 log entries during its life cycle
  • log entry payload usually < 20 fields, total size < 1KB

benchmark shows we will be better with sub-logger if request leaves 100+ entries during its life cycle, in our current production env, the sub-logger is about at least 1000X worse in regard of memory signature

The only benefits might be alloc per request, which is trivial to performance according to prod metrics, it might have reduced GC but the benefit is pretty hard to measure.

In conclusion: we will not do per request sub logger, we'll be extracting data we need from context, the other problem that I'm not addressing here is where the fields comes from (maybe up to 50 fields from both context and headers). I'll close this PR since it's proved sub-logger is worse, I'm going to open another PR to populate requestUUID to loggers

@cl1337
Copy link
Contributor Author

cl1337 commented Aug 9, 2018

cc @ravirajj as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants