Add support for tracing / logging

[damianh]

No external dependencies allowed. Should allow passing in a Action / delegate to that can handle a trace / log message. User would have adapt that to the application logger or MS.Owin.Logging in their startup.

Thinking something like

app.MaxBandwidth(10000, (logLevel, message) => { ... });

[StefanOssendorf]

Like

environment => 
{
    // Do loggin stuff
}

? e.g.

builder.MaxUrlLength(() => 12, env => { /* logging impl */ });

The most important question is, where to set the logger calls an how to provide more context. I think the OWIN env is not enough?

[damianh]

Don't think I want to expose the entire environment like that. Seems kinda dangerous / leaky...

[StefanOssendorf]

Right, but what's "enough" context to get a good tracing/logging mechanism?

[damianh]

I think it depends on on context of the middleware, but for our case something simple should be sufficient. Here is MS.Owin.Logging.ILogger http://katanaproject.codeplex.com/SourceControl/latest#src/Microsoft.Owin/Logging/ILogger.csPerhaps we just need a Action<TraceEventType, string> ? On 3 Apr 2014 21:25, "Stefan Ossendorf" notifications@github.com wrote:

Right, but what's "enough" context to get a good tracing/logging mechanism?

Reply to this email directly or view it on GitHubhttps://github.com/owin-middleware/Owin.Limits/issues/8#issuecomment-39493927 .

[StefanOssendorf]

Mhm... I miss something. What's the benefit for the user when we implement this?

[damianh]

No, I'm convinced there is huge benefit tbh. I think it would be nice to trace when the middleware decides to intercept the response. Let's sit on this issue a bit, chew it over.

If we decide to go ahead this, another thought...

Am also thinking that I'd like to make the reason phrases configurable, so rather then overloading the UseX extension method, we could use an options object (like Nancy https://github.com/NancyFx/Nancy/blob/master/src/Nancy.Owin/NancyOptions.cs )

How does this look, shortened for berevity?

public class MaxRequestLengthContentOptions
{
    public MaxRequestLengthContentOptions(int maxContentLength)
    {
    }

    public Action<TraceEventType, string> Logger { get; set; }

    public Func<int, string> LimitReachedReasonPhrase { get; set; }

    ....
}

// leave the other overloads as they are but they can be delegated
// to this options ext method
app.MaxRequestContentLength(new MaxRequestLengthContentOptions(1000) { Logger = (eventType, message) => { ... } );

Think a spike may be in order...

[StefanOssendorf]

Hi! Something like that (Traceing)? https://github.com/StefanOssendorf/Owin.Limits/blob/TracingSpike/src/Owin.Limits/MaxQueryStringLengthMiddleware.cs

[StefanOssendorf]

I like those Option/Configuration-Objects. It's much easier to expand compared to method overloads.

[damianh]

That trace spike looks good. Probably wouldn't have the Trace private method, just invoke the action directly.

[StefanOssendorf]

I'm lazy. It's annoying to write TraceEventType.Information. If you don't mind I'll keep that private method for easier writing but should it be logged as Information or Verbose Type?

[damianh]

Probably different different types depending on what happens, hence my original statement on the private method.

Verbose for messages when the limit is not breached. Informational if the limit is breached.

On 7 Apr 2014 19:05, "Stefan Ossendorf" notifications@github.com wrote:

I'm lazy. It's annoying to write TraceEventType.Information. If you don't mind I'll keep that private method for easier writing but should it be logged as Information or Verbose Type?

Reply to this email directly or view it on GitHub.

[StefanOssendorf]

First commit: https://github.com/StefanOssendorf/Owin.Limits/commit/a8050726e4b69784ee120635ddbfc1d7a40a0660

PR when finished.

[StefanOssendorf]

I'm thinking about adding a GUID in each trace message to identify messages from one Request with Response. What do you think?

[damianh]

Yeah there is nothing in the owin spec to identify a request to help with things like correlation of log output. Loggers also have things called nested diagnostic contexts that work great for multithreaded / concurrent apps. Ideally you'd want to have the same 'identifier' for all tracing across all middleware.

Let me ping my owin buddies on this. It'll be tomorrow before I get around to it. On 10 Apr 2014 20:59, "Stefan Ossendorf" notifications@github.com wrote:

I'm thinking about adding a GUID in each trace message to identify messages from one Request with Response. What do you think?

Reply to this email directly or view it on GitHubhttps://github.com/owin-middleware/Owin.Limits/issues/8#issuecomment-40124619 .

[damianh]

Regarding GUID in log messages - don't think we should do that but leave it up to the logging implementation to provide the correct context for correlation purposes (i.e. a nested diagnostic context).

[damianh]

@StefanOssendorf How I would expect a logging framework to do the correlation on our behalf https://github.com/damianh/Serilog.Owin/blob/master/src/Serilog.Owin/SerilogMiddleware.cs#L28

[StefanOssendorf]

@damianh Okay. I'll leave it.

[damianh]

This is how I imagine it might look like: https://github.com/damianh/Serilog.Owin/blob/master/src/Serilog.Owin/SerilogMiddleware.cs On 12 Apr 2014 14:30, "Stefan Ossendorf" notifications@github.com wrote:

@damianh https://github.com/damianh Okay. I'll leave it.

Reply to this email directly or view it on GitHubhttps://github.com/owin-middleware/Owin.Limits/issues/8#issuecomment-40279173 .

[StefanOssendorf]

Yes? I can't follow, sorry.

[damianh]

It's a serilog middleware to open a nested diagnostic context to correlate log messages from a request. On 12 Apr 2014 15:06, "Stefan Ossendorf" notifications@github.com wrote:

Yes? I can't follow, sorry.

Reply to this email directly or view it on GitHubhttps://github.com/owin-middleware/Owin.Limits/issues/8#issuecomment-40279893 .

[StefanOssendorf]

Yes, so we stick with Action<TraceEventType,string> or are you thinking of replacing string with a context object?

[damianh]

Stick with Action On 12 Apr 2014 15:20, "Stefan Ossendorf" notifications@github.com wrote:

Yes, so we stick with Action or are you thinking of replacing string with a context object?

Reply to this email directly or view it on GitHubhttps://github.com/owin-middleware/Owin.Limits/issues/8#issuecomment-40280165 .

[StefanOssendorf]

Okay, now i'm clear in my head. ;-) Thanks for your patience.

[StefanOssendorf]

issue solved/implemented? :-)

[damianh]

Yes it is :)