Added route filtering for Owin and AspNetCore middleware #229
Conversation
| => appBuilder.Use<ZipkinMiddleware>(serviceName, traceExtractor, getRpc); | ||
| public static IAppBuilder UseZipkinTracer(this IAppBuilder appBuilder, string serviceName, IExtractor<IHeaderDictionary> traceExtractor, Func<IOwinContext, string> getRpc = null, | ||
| Func<PathString, bool> routeFilter = null) | ||
| => appBuilder.Use<ZipkinMiddleware>(serviceName, traceExtractor, getRpc, routeFilter); |
There was a problem hiding this comment.
@bstauff Can we update the line limit to at most 120 so that it will be easier to view all the arguments? No need to scroll.
Please apply it on the method below too.
|
@bstauff Thanks for putting this in. This would be helpful to our team and to others I think. Overall , implementation looks good! |
|
Added a unit test for the owin middleware. |
| @@ -20,15 +23,18 @@ public static class TracingMiddleware | |||
|
|
|||
| var trace = traceContext == null ? Trace.Create() : Trace.CreateFromId(traceContext); | |||
| Trace.Current = trace; | |||
| using (var serverTrace = new ServerTrace(serviceName, getRpc(context))) | |||
| if (routeFilter(request.Path)) | |||
There was a problem hiding this comment.
Looking at the new changes with unit test (thanks for adding), I think you need to add else condition here too.
There was a problem hiding this comment.
this is the only blocking change, let's get this in an @bstauff publish!
There was a problem hiding this comment.
Sorry for the delay! Got really busy. I've made the changes, thanks for the catch!
There was a problem hiding this comment.
LGTM! Thanks a lot @bstauff
I was wondering if we maintain a readme for using middlewares for both dotnet core and owin. We can add it in issues as a TODO list and be picked up by anyone to contribute.
|
Thanks! |
I modified the Owin and AspNetCore middleware to support a func for route filtering, as requested in #228