mdsol / Medidata.ZipkinTracerModule

[Deprecated] Zipkin Request Tracing for .Net Apps
MIT License
53 stars 20 forks source link

Replace log4net dependency with LibLog #83

Closed petemounce closed 8 years ago

petemounce commented 8 years ago

This allows consumers to use any log library supported by LibLog, rather than requiring a log4net dependency.

jcarres-mdsol commented 8 years ago

@bvillanueva-mdsol @lschreck-mdsol @kenyamat Please review

lschreck-mdsol commented 8 years ago

This liblog is a very smart solution for a package-independent logging!

The PR looks good to me! :+1:

bvillanueva-mdsol commented 8 years ago

@petemounce This looks good! Can you add a note in the readme to indicate LibLog is in the works for this module? Also, please update the readme for sample usage that involves logging as a parameter in constructor for ZipkinClient. Thanks!

petemounce commented 8 years ago

Hi - thanks. Done in c61260c. Logging is no longer a required parameter to the ZipkinClient because loggers are created internally now at need via the LogProvider.GetCurrentClassLogger() method which also names them to their type's fully qualified name (which in turn allows consumers more control over which logs they listen to).

petemounce commented 8 years ago

@bvillanueva-mdsol @lbeckman-mdsol @jcarres-mdsol anything else you'd like me to do in this PR? I'd love to get this merged and published - next week @justeat has a hackathon and I'm hoping to be able to introduce zipkin to our .NET platform.

lbeckman-mdsol commented 8 years ago

@petemounce I'm not in a strong position to respond, but I don't (initially) see any big issues with using zipkin.

jcarres-mdsol commented 8 years ago

@petemounce sorry for the slow response. Last week we had a holiday day and could not look at this till now. I think it is good, when these PRs get merge we can update the version if that's good for @bvillanueva-mdsol @lschreck-mdsol @kenyamat

lschreck-mdsol commented 8 years ago

I think it is good, so if @kenyamat thinks it is OK, we can merge!

bvillanueva-mdsol commented 8 years ago

@petemounce Hello, Im really sorry for the late response on this PR and the other PRs. We were tied up in our recent sprint and it is our (team) first time to handle request on our open source projects here in github. Starting from now, for the following two weeks, I'll try my best and our team to respond to PRs like this. I understand you need the PR to be merged and publish soonest. Unfortunately, Im out of of the office tomorrow (im replying from home.), I'll work on this on Friday. Thank you!

petemounce commented 8 years ago

@bvillanueva-mdsol that's ok - in the meantime I've vendored it and will contribute back as I go. One of the changes was to make it possible to add more binary annotations based on the IOwinContext to, for example, grab our own metadata headers onto the spans - but, by adding a virtual method onto IZipKinConfig so that consumers can override.

bvillanueva-mdsol commented 8 years ago

Tested and looks good. Logging work with our application using log4net. Thanks @petemounce for this PR. Merging it then will publish.

bvillanueva-mdsol commented 8 years ago

@petemounce Please post a PR on the changes about adding more binary annotations based on the IOwinContext. Looks interesting FYI @jcarres-mdsol

petemounce commented 8 years ago

Thanks for the merge, other changes incoming.