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

Feature/17 logging #80

Merged
merged 26 commits into from
Apr 18, 2014
Merged

Feature/17 logging #80

merged 26 commits into from
Apr 18, 2014

Conversation

haf
Copy link
Contributor

@haf haf commented Apr 17, 2014

A PR that implements replaceable logging, common abstraction points and cleans up code that the replacement touched. Also implements some header parsing to track span ids flowing from clients.

Fixes #17 .

@haf
Copy link
Contributor Author

haf commented Apr 17, 2014

In this PR it's a good idea to read the code with whitespace diffs turned off: https://github.com/SuaveIO/suave/pull/80/files?w=1

haf added 8 commits April 17, 2014 19:28
If we always read all requests fully into memory, we can get
DoS-attacked by just posting large data requests to the server and we
might want to perform a Content-Length header check before reading all
that data.

By performing the header check, we can respond with 100 Continue if it's
a valid length/something we accept. This will also work better in proxy
mode when we make more of the request contents available to the proxy,
since it would like to read piecemeal from the request input stream and
write to the upstream server.
@haf
Copy link
Contributor Author

haf commented Apr 17, 2014

The code in Web.fs could use some unit tests, really. It's quite opaque code there and with the performance related changes to mutable collections it's hard to reason about.

@haf
Copy link
Contributor Author

haf commented Apr 17, 2014

So what did I learn doing this? 1) avoid having any global state, just pass the state along the functions: it's slightly more annoying right now, but it's vastly easier after a while, 2) the tracing functionality doesn't make much sense if we're outside the scope of a proper web request, and then I simply pass empty trace data to the logger

@ademar
Copy link
Member

ademar commented Apr 18, 2014

Looks great Henrik, go ahead and merge it.

I had a question, would it make sense to get rid of the lock by sending the messages to a MailboxProcessor ?

@haf
Copy link
Contributor Author

haf commented Apr 18, 2014

Thanks. I want to write a few lines of docs on how to implement some logging, too, because that's the first question that will come from the users.

About the lock (in ConsoleWriter I suppose): yes! But no! :) I plan on providing extension libraries for logging that does exactly this - implements better targets. I want this to be the most simple basic implementation that triggers people to experiment -- after all it's just a single method to implement and everything else is local to the LogLine -- so in comparison to how much boilerplate NLog and log4net has, this should be easier.

@haf
Copy link
Contributor Author

haf commented Apr 18, 2014

I'm reading a bit of web machine docs because I think that a resource-oriented layer on top of suave is the way to go for the vast majority of web APIs out there. It has a nice feature of returning {{trace, Dir}, Context} to make a resource go into debugging mode. This is similar to what we had with verbose_logging previously but on an input-output level instead of globals.

@haf
Copy link
Contributor Author

haf commented Apr 18, 2014

Added docs in 5524754

haf added a commit that referenced this pull request Apr 18, 2014
@haf haf merged commit d6da64f into master Apr 18, 2014
@haf haf deleted the feature/17-logging branch April 18, 2014 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the logging to be replaceable and configurable
2 participants