sourcegraph / appdash

Application tracing system for Go, based on Google's Dapper.
https://sourcegraph.com
Other
1.72k stars 137 forks source link

Add appdash/nettrace package #48

Closed tcolgate closed 8 years ago

tcolgate commented 9 years ago

First draft at tracer for adding events for an underlying net.Conn Still needs tests and documentation, but I'd like to know if you'd be interested in it if I tidy it up.

I'm using this as part of a benchmarking client for an API, which I realise is a bit of an odd use of appdash (it's all client side), but the code was easy to write and it seemed like it could be generally useful.

Feedback is welcome.

slimsag commented 9 years ago

Sounds like a reasonably well-fitted feature for Appdash. I have a few requests if you have the time:

Thanks for the PR! :smile:

tcolgate commented 9 years ago

I've split the Dial and net.Conn out into seperate files, and renamed as requested. I'll have a crack at writing some tests at the end of the week. (I've never got the hang of unit testing this kind of stuff so it'll be a good thing for me to do). I've got code using this in an internal project, so putting together an eample app should be pretty easy.

slimsag commented 9 years ago

Hey @tcolgate -- great work! I really like this! If you want any help or feedback on writing the tests just let me know, I'd be glad to help.

I added a bunch of comments on the first file regarding // comment style -- but they're very minor (and I can adjust them after merging if you don't want to). They also apply to the ConnWriteEvent type (the other file).

If you could put together a example app I would be very happy! If not just let me know, and I'll see if I can write one soon to verify that this API works pretty well.

Looking forward to your progress on this and to merging it! :smile:

tcolgate commented 9 years ago

Sorry this took a while, not had much spare time. Updated the comments, and removed the two NewConn(Read|Write)Event. Still need a unit test.

slimsag commented 9 years ago

Apologies for the late review. The code LGTM.

If you don't have time to write a unit test (or don't want to) I can merge this as-is and add one myself -- just let me know!

We really appreciate the PR! :smile:

tcolgate commented 9 years ago

@slimsag cheers, I'm going to hve a crack at the unit test. It'll be good practice.

tcolgate commented 9 years ago

feel free to merge first though, if you want to make it available.It might be a while before the test is ready (not got a lot of spare time in the next week or so, I will try today, if I don't get it done, it may be another week)

tcolgate commented 9 years ago

@slimsag I thought I'd update this and have a crack at adding some tests. Nothing too mind blowing yet, but I'll try and work on it. Will add Close() tracing too I think. I don't actually need this code any more, but it's fun to work on.

slimsag commented 9 years ago

@tcolgate Awesome! I appreciate it! If you ever feel like not working on it let me know and I can take over it. I think this will make a really good example on how Appdash can work in many areas, not just in HTTP applications :)

slimsag commented 8 years ago

I'm going to close this for now because it is stale. Once someone (incl. me) is ready to work on it, feel free to re-open / comment.

FWIW I do think this is a good change that we should make long-term.