Upgrade to zap 1.0 release candidate #290

akshayjshah · 2017-02-18T08:55:43Z

This PR upgrades UberFX to use the release candidate of zap 1.0. Along the way, it migrates FX to use the base zap logger (what you were calling the typed logger). It includes a few potentially controversial changes, which I'll address head-on here. I'm happy to chat about any of this in person on Monday.

(Clearly, this can't get merged until after your code freeze. That's cool, I'd just like to get review done before I'm constantly rebasing.)

No ulog.Log interface

Interfaces are good for a few things:

depending on a subset of a type's methods
allowing callers to pass third-party types without depending on those external packages
abstracting away implementation, which lets you swap implementations later

However, the former ulog.Log interface doesn't accomplish any of these goals. It would have included the full zap.SugaredLogger API, and it doesn't prevent UberFX (and ulog) from taking a concrete dependency on zap. Most importantly, because we need users to be able to recover the strongly-typed *zap.Logger, it references tons of concrete zap types (*zap.Logger, which then references Field and *CheckedEntry). It's hard for me to imagine a reimplementation of that API outside zap.

That might still be okay, but interfaces are also harder to extend - adding even a single method is a breaking change. Given all that, it seems wiser to me to use *zap.Logger directly and eliminate a bunch of copypasta. Since we own both libraries, I don't see this as a significant problem. - users can and should declare narrow interfaces for just the portions of the logger that they're using.

No Sentry integration

This is purely a time issue. If you can't wait for the Filebeat plugin we've discussed, I'm happy to resurrect direct Sentry integration in a follow-on PR. The latest zap should allow us to do that without so many map copies and with zero impact on non-error log performance.

No loggers in contexts

I was a little confused by the existing code. It looks to me like we're largely working with a single global logger, which starts off with some default configuration and is reconfigured on service startup. We're supplying that logger to users by putting it into the request context and providing ulog.Logger(ctx context.Context) ulog.Logger as a getter. Where possible, we add tracing information to the logger before putting it into the context.

In all that, I'm not sure whether the logger needs to be in the context at all. We can keep ulog.Logger, and add just add the context's tracing information to the logger when it's retrieved. This is quite inexpensive, and it's much simpler.

No log builder

I don't think that this is required any more either; we can just use zap's config shapes instead. They already come with sensible default values, and we can modify the service scaffold to comply with our internal logging schemas.

Documentation

I didn't touch the ulog README much - I'll leave that for someone on your team.

mention-bot · 2017-02-18T08:55:44Z

@akshayjshah, thanks for your PR! By analyzing the history of the files in this pull request, we identified @anuptalwalkar, @glibsm and @alsamylkin to be potential reviewers.

akshayjshah · 2017-02-18T09:05:15Z

Argh, there's already a massive conflict with Anup's latest logging refactoring. I'll resolve all the conflicts later this weekend.

ascandella

Notes, mostly for myself:

This removes IsDevelopmentEnvironment()
Will require internal documentation changes
Need to discuss our story around exposing tally directly versus a duck-typed interface and whether or not we want to do the same (and the associated overhead)
Timelines

Generally I'm +2 on this change and willing to merge/manually test myself tomorrow if we can iron out the major issues today/tomorrow.

I still have this lingering feeling we want a facade interface though, having seen how many times users have been bitten by library upgrades. Not saying that your team will do that, but whether my team should take a hard stance and do our best to never expose types we don't control directly as part of our APIs.

ascandella · 2017-02-18T16:33:00Z

auth/uauth.go

@@ -48,7 +48,7 @@ var (
 // CreateAuthInfo interface provides necessary data
 type CreateAuthInfo interface {
 	Config() config.Provider
-	Logger() ulog.Log
+	Logger() *zap.Logger


I think this is going to be the most contentious part cc @shawnburke who deals with the same issue in Catalyst. I'm sure your 1.x series will maintain API compatibility but we've been trying to introduce as many shim layers as possible (caveat: you'll see nearby we're exposing tally directly, which we may decide not to) to provide you the flexibility to change your APIs without worrying about breaking UberFx consumers.

Let's discuss this in person. I Have a lot of reservations about top-level frameworks own interfaces for logging and metrics; it forces components far down the stack to depend on the framework, instead of the reverse.

Meant to post to this comment, but replied to the email instead so it's at the bottom. Copy-paste:

Ya, this is definitely not black-and-white, definitely should hammer out in
person. My primary concern is stability, not ownership. If we can keep
pushing new versions of Fx without ever breaking people using our logging
mechanism, then my vote is for whatever mechanism makes that easiest.

I started email thread before this PR, and Im curious too. I have been asked this question multiple times that why fx is so tightly integrated with zap.Logger, and can it be used with different logger implementation.

Ack ya I am starting to really like zap, and am super impressed with it, but...I have to agree, I hate when frameworks/libraries tightly link a logging library. I wish we could even document the Typed() function to return nil if zap is not being used, or something a little less unsafe.

ascandella · 2017-02-18T16:33:57Z

examples/keyvalue/server/observer.go

@@ -32,7 +32,7 @@ type Observer struct {

 // OnInit is called during service init process. Returning an error halts the init?
 func (o *Observer) OnInit(svc service.Host) error {
-	svc.Logger().Info(
+	svc.Logger().Sugar().Infow(


Sugar() is low enough overhead/memoized that we don't need a svc.SugaredLogger()? Cool!

Yep, its one allocation. Not free, but not a big deal if you're using the sugared API. If UberFx gets to a point where it's counting single allocations, memorizing this may make sense.

How do you feel about svc.TypedLogger() and svc.Logger() being sugared? I'm on the fence as to how we want to expose this, but my gut (and my discussions outside of uber) is that most people find the verbosity of zap.String() etc too high, and will opt for typed only when perf is a concern.

Let's defer pending the interface discussion. If we end up not putting a facade in front of logging, we can do away with this API altogether - it's just a roundabout way of giving people access to zap.L and zap.S, which FX startup configures.

ascandella · 2017-02-18T16:34:42Z

glide.yaml

-  # Pinned temporarily while zap reaches 1.0 (https://github.com/uber-go/fx/pull/243)
-  version: c064b5c44b285a7e2fd5c9b26e8c38228ce2bccb
+- package: go.uber.org/zap
+  version: v1.0.0-rc.1


Any release timeline on 1.0? We had an incredibly painful amount of churn with yarpc 1.0 RC candidates, both for my team and our consumers.

Typical for an RC - depends on how many things I got wrong :)

I'm hoping to update some other internal projects this week and cut a 1.0 final before the end of the month. The APIs for the loggers themselves are very, very unlikely to change; more likely that some portion of the constructors changes.

I misspelled a JSON key, so there's and RC.2 coming Monday. Shouldn't affect you guys.

Cool. Again though, this makes me think we should keep a ulog.Logger even if it's currently just the exact same interface as a zap.Logger so that we can upgrade to v1 without breaking users.

Any breaking change in the RC will also break your interface, so I'm not sure what this buys you.

I've had this discussion about interfaces with Peter and Josh before, and it's nuanced enough that we should take it out of PR comments. I'll send you a short proposal, and we can discuss in person. I'm certainly not blocking you from shipping what you want in FX, but I don't think that this interface will be helpful.

ascandella · 2017-02-18T16:35:27Z

modules/rpc/handler.go

@@ -85,7 +89,7 @@ func (a authOnewayInboundMiddleware) HandleOneway(ctx context.Context, req *tran
 func authorize(ctx context.Context, host service.Host) (context.Context, error) {
 	if err := host.AuthClient().Authorize(ctx); err != nil {
 		stats.RPCAuthFailCounter.Inc(1)
-		ulog.Logger(ctx).Error(auth.ErrAuthorization, "error", err)
+		ulog.Logger(ctx).Error(auth.ErrAuthorization, zap.Error(err))


Oh man, @akshayjshah, you really went above and beyond here. We have an action item to convert to unsugared usage internally. Really appreciate this!

ascandella · 2017-02-18T16:35:52Z

modules/rpc/handler.go

@@ -110,7 +114,7 @@ func (p panicOnewayInboundMiddleware) HandleOneway(ctx context.Context, req *tra
 func panicRecovery(ctx context.Context) {
 	if err := recover(); err != nil {
 		stats.RPCPanicCounter.Inc(1)
-		ulog.Logger(ctx).Error("Panic recovered serving request", "error", errors.Errorf("panic in handler: %+v", err))
+		ulog.Logger(ctx).Error("Panic recovered serving request.", zap.Any("error", err))


Is this a thing? Periods after messages? If so, we should document somewhere and communicate with my team.

Sorry, broke with your convention out of habit. My strong preference is for comments and log messages to be complete sentences - initial capitals, proper spelling and grammar, and terminal punctuation. I think I'm out of the mainstream on this though.

Golang comments expect a period in the end: https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences
I have seen we missed period in a few places.

Comments !== log messages

I think I'm agreeing with Aiden 2-0 so far in the Aiden/Akshay "Battle of the PR" here. I actually don't even like sentences, I like "Events", and wish they were somehow documented. Think request_panic_recovered.

Sure. Don't think that this is a place we need consensus.

ascandella · 2017-02-18T16:39:44Z

service/host.go

 		}
 	}

 	// Flush tracing buffers
 	if s.tracerCloser != nil {
 		s.Logger().Debug("Closing tracer")
 		if err = s.tracerCloser.Close(); err != nil {
-			s.Logger().Error("Failure to close tracer", "error", err)
+			s.Logger().Error("Failure to close tracer", zap.Error(err))


TODO add period here?

ascandella · 2017-02-18T16:39:53Z

service/host.go

@@ -199,7 +206,7 @@ func (s *host) AddModules(modules ...ModuleCreateFunc) error {
 		if !s.supportsRole(mi.Roles...) {
 			s.Logger().Info(
 				"module will not be added due to selected roles",


TODO(my team) capitalize "Module"

ascandella · 2017-02-18T16:40:11Z

service/host.go

@@ -278,9 +285,12 @@ func (s *host) start() Control {

 				s.shutdownMu.Unlock()
 				if _, err := s.shutdown(e, "", nil); err != nil {
-					s.Logger().Error("Unable to shut down modules", "initialError", e, "shutdownError", err)
+					s.Logger().Error("Unable to shut down modules",


No period? Perhaps I'm misunderstanding the guideline here.

ascandella · 2017-02-18T16:40:45Z

service/host.go

-					s.Logger().Error("Unable to shut down modules", "initialError", e, "shutdownError", err)
+					s.Logger().Error("Unable to shut down modules",
+						zap.String("initialError", e.Error()),
+						zap.String("shutdownError", err.Error()),


We're going to lose the cause/stacktrace of these errors by turning them into Strings, right?

Possibly. I'll take another pass to make sure we don't lose any info.

I'm not going to take a dependency on Daves error package in zap (it's pre-1.0), but I should still be able to support these errors with Error.

ascandella · 2017-02-18T16:41:20Z

service/host.go

@@ -400,7 +413,11 @@ func (s *host) transitionState(to State) {

 	// TODO(ai) this isn't used yet
 	if to < s.state {
-		s.Logger().Fatal("Can't down from state", "from", s.state, "to", to, "service", s.Name())
+		s.Logger().Fatal("Can't down from state",


Again, same question about periods. Seems like maybe we should just leave them out and back out the changes towards the top to remain consistent

ascandella · 2017-02-18T23:45:33Z

With regards to your comment:

No loggers in contexts

I was a little confused by the existing code. It looks to me like we're largely working with a single global logger, which starts off with some default configuration and is reconfigured on service startup. We're supplying that logger to users by putting it into the request context and providing ulog.Logger(ctx context.Context) ulog.Logger as a getter. Where possible, we add tracing information to the logger before putting it into the context.

In all that, I'm not sure whether the logger needs to be in the context at all. We can keep ulog.Logger, and add just add the context's tracing information to the logger when it's retrieved. This is quite inexpensive, and it's much simpler.

It's always seemed weird to me that the logger object was stored as a value on the context. If we can just extract the tracing when we need it, it seems much cleaner to not add additional metadata. I tried to work this out with @anuptalwalkar and @alsamylkin, but they had some technical reasons why it was necessary to store on the context.

akshayjshah · 2017-02-19T00:21:30Z

I'm out on Monday, but will try to get the remaining nits ironed out by Tuesday. Mind scheduling something for us to chat Tuesday morning?

ascandella · 2017-02-19T00:27:15Z

Sure. What's the scope? Fascade interfaces in general, review of this PR, scheduling/timing/ownership?

…

On Sat, Feb 18, 2017 at 4:21 PM Akshay Shah ***@***.***> wrote: I'm out on Monday, but will try to get the remaining nits ironed out by Tuesday. Mind scheduling something for us to chat Tuesday morning? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#290 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P-ipBYpbe0GYIxIPJ4ED5D3G84DFks5rd4sKgaJpZM4MFDXz> .

ascandella · 2017-02-19T00:31:04Z

cc @shawnburke we should (maybe?) take a consistent stance on fascade interfaces between UberFx and Catalyst, otherwise we could end up with some painful conversion functions. Would you like to join the meeting, or have a separate discussion w.r.t. UberFx+Catalyst once we've figured out our strategy on zap/tally/etc?

ascandella · 2017-02-19T00:31:40Z

Also lolin at the merge conflicts. For such a small codebase, it's pretty amazing how bad this conflicted:

glibsm · 2017-02-19T02:15:28Z

modules/uhttp/client/client_filters_test.go

@@ -74,7 +74,7 @@ func TestExecutionChainFiltersError(t *testing.T) {
 }

 func withOpentracingSetup(t *testing.T, registerFunc auth.RegisterFunc, fn func(tracer opentracing.Tracer)) {
-	tracer, closer, err := tracing.InitGlobalTracer(&jconfig.Configuration{}, "Test", ulog.NopLogger, metrics.NopCachedStatsReporter)
+	tracer, closer, err := tracing.InitGlobalTracer(&jconfig.Configuration{}, "Test", zap.New(nil), metrics.NopCachedStatsReporter)


Consider zap.Nop() instead of zap.New(nil) for NopCore.
I actually had to pull up zap to see what it meant to pass in nil

Yep, fair point. New supports nil purely for safety (since I can't stop people from passing nil interfaces, this seemed like the best way to not add an error return). uber-go/zap#326 adds an explicit NewNop.

akshayjshah · 2017-02-19T17:51:04Z

Sure. What's the scope? Fascade interfaces in general, review of this PR,
scheduling/timing/ownership?

I'd definitely like to get through this PR, and ideally also figure out the facades & interface ownership aspects.

anuptalwalkar · 2017-02-19T19:26:28Z

modules/uhttp/filters.go

@@ -55,8 +55,7 @@ type contextFilter struct {
 }

 func (f contextFilter) Apply(w http.ResponseWriter, r *http.Request, next http.Handler) {
-	ctx := ulog.NewLogContext(r.Context())
-	next.ServeHTTP(w, r.WithContext(ctx))
+	next.ServeHTTP(w, r)


I want to keep this. We had this filter to keep the logger inside context and making it available for users. Primary reason is to keep adding fields that are not provided by zap. Logger(ctx) provides default fields from zap configuration (host, time, etc), but some of the fields come from fx. Right now, you have module: "http", service name, traceID, spanID with the logger available in the user handler.

To answer the other question on why Logger is on the context and not fields. I see that you have added Logger(ctx) to pull all fields from context instead of returning configured logger. This seems efficient right now, but it will be O(n) operation n being number of fields in the context to make the logger rich enough for our needs. There is this slight performance benefit of having Logger in the context since most of the codebase is literally doing 'ulog.Logger(ctx)' on every logging step. What do you think? We can comeback to perf later since either way will not break any APIs.

anuptalwalkar · 2017-02-19T19:30:38Z

ulog/log.go

+// hijack output from the standard library's "log" package. It returns a
+// function to undo these changes.
+func SetLogger(log *zap.Logger) func() {
+	undoGlobals := zap.ReplaceGlobals(log)


This is sweet!

anuptalwalkar · 2017-02-19T19:40:55Z

auth/uauth.go

@@ -48,7 +48,7 @@ var (
 // CreateAuthInfo interface provides necessary data
 type CreateAuthInfo interface {
 	Config() config.Provider
-	Logger() ulog.Log
+	Logger() *zap.Logger


I started email thread before this PR, and Im curious too. I have been asked this question multiple times that why fx is so tightly integrated with zap.Logger, and can it be used with different logger implementation.

bufdev · 2017-02-20T16:56:30Z

I really really really like this PR. This is going to break like, everything, ever, but I don't mind at all. Totally worth it.

akshayjshah · 2017-02-20T20:03:45Z

I suspect that, at the end of the day, a bunch of the migration pain can be fixed with gofmt -r rules. I'll play with it a bit and write a little script for users.

akshayjshah · 2017-02-23T09:53:42Z

Rebased on master, addressed some nits. This go-round leaves all log messages exactly as-is :)

Travis will fail until I can get some eyes on uber-go/zap#333 and uber-go/zap#329. Once those land, I can cut a second RC and update FX's Glide manifest and lock files.

ghost

Like the ratio: 1980 lines removed, 469 added

ghost · 2017-02-23T19:38:05Z

ulog/context.go

+	}
+	j, ok := span.Context().(jaeger.SpanContext)
+	if !ok {
+		return nil


This looks weird..

Can you elaborate a bit? If there's no Jaeger span available, there's nothing to do; it's not an error, it's an expected case.

I am confused, why is it an expected case?

ghost · 2017-02-23T19:44:46Z

ulog/metrics.go

+// To register it with a zap.Logger, use zap.Hooks.
+func Metrics(s tally.Scope) func(zapcore.Entry) error {
+	// Avoid allocating strings and maps in the request path.
+	debugC := s.Tagged(map[string]string{"level": "debug"}).Counter("logs")


can we simplify it even further and put counters in a map, so returning function will probably have only one if?

This is faster - why take on a hash map lookup when we can use a short switch instead? For straightforward, well-encapsulated, often-executed code, I don't think that the brevity gains are worth it.

Of course, y'all own this code - I'll change if you really want me to.

Well, this is because Zap has weird constants :) otherwise we can use arrays and be on par.
I did a simple benchmark and maps are 3 times slower, so it is good to keep switch.

akshayjshah

Cool - thanks for the stamp! The add/delete ratio is unfortunately misleading, since this diff drops Sentry support :/

We'll need to get that filebeat extension shipped pronto.

akshayjshah · 2017-02-23T20:05:55Z

ulog/context.go

+	}
+	j, ok := span.Context().(jaeger.SpanContext)
+	if !ok {
+		return nil


Can you elaborate a bit? If there's no Jaeger span available, there's nothing to do; it's not an error, it's an expected case.

akshayjshah · 2017-02-23T20:08:36Z

ulog/metrics.go

+// To register it with a zap.Logger, use zap.Hooks.
+func Metrics(s tally.Scope) func(zapcore.Entry) error {
+	// Avoid allocating strings and maps in the request path.
+	debugC := s.Tagged(map[string]string{"level": "debug"}).Counter("logs")


This is faster - why take on a hash map lookup when we can use a short switch instead? For straightforward, well-encapsulated, often-executed code, I don't think that the brevity gains are worth it.

Of course, y'all own this code - I'll change if you really want me to.

ghost · 2017-02-23T22:54:47Z

My stamp is not binding to merge, I agree with the changes, but we need to figure out sentry(and fix lint errors) to merge.

This necessarily updates all the other dependencies, too.

Remove most of ulog, since it's either unnecessary or subsumed by zap. The only lost functionality here is Sentry integration, which we can bring back next week (either as an UberFX-specific encoder wrapper, or as a Filebeat plugin).

There's no need for this; we don't need to put loggers into the context in order to add tracing information to the context.

Again, this necessarily upgrades all other dependencies.

coveralls · 2017-02-24T00:30:43Z

Coverage decreased (-1.6%) to 90.63% when pulling 80c2077 on ajs-zap into 62a9f89 on master.

anuptalwalkar · 2017-02-24T00:35:45Z

tracing/tracer_test.go

 	assert.NotNil(t, jConfig)
 	assert.Equal(t, jaeger.NullLogger, jConfig.Logger)
 }

 func TestLoadAppConfig_NilJaegerConfig(t *testing.T) {
-	jConfig := loadAppConfig(nil, getLogger())
+	jConfig := loadAppConfig(nil, zap.L())


zap.L and zap.S are a bit confusing. Not blocking this PR, but we may need some verbose method here.

Yep, these are extremely short by design. Many logging packages have top-level Error, Infof, etc. functions; I really, really don't want these in zap, which means that we need a minimally noisy way to get the global logger. zap.GlobalLogger().Info("something") seems like a lot; zap.L().Info("something") is much less intrusive.

This is definitely a matter of taste, though - file an issue and we can discuss pros and cons there.

anuptalwalkar · 2017-02-24T00:54:09Z

Merging the PR, since conflicts are resolved here and offline.

ascandella · 2017-02-24T01:00:42Z

❤️

glibsm · 2017-02-24T01:10:35Z

leerooooooooooyyyyyyyyy

ascandella reviewed Feb 18, 2017

View reviewed changes

glibsm reviewed Feb 19, 2017

View reviewed changes

anuptalwalkar reviewed Feb 19, 2017

View reviewed changes

anuptalwalkar mentioned this pull request Feb 20, 2017

Update zap to latest commit on dev branch #243

Closed

glibsm added the refactor label Feb 21, 2017

akshayjshah force-pushed the ajs-zap branch from cf21bb8 to 5b09b2b Compare February 23, 2017 09:50

ghost approved these changes Feb 23, 2017

View reviewed changes

akshayjshah commented Feb 23, 2017

View reviewed changes

Akshay Shah added 10 commits February 23, 2017 16:12

Add zap 1.0.0-rc.1 to Glide manifest

d857907

This necessarily updates all the other dependencies, too.

Update ulog to the latest zap

c877139

Remove most of ulog, since it's either unnecessary or subsumed by zap. The only lost functionality here is Sentry integration, which we can bring back next week (either as an UberFX-specific encoder wrapper, or as a Filebeat plugin).

Remove internal context key

10fcddd

There's no need for this; we don't need to put loggers into the context in order to add tracing information to the context.

Migrate testutils to zap RC

2c20ce6

Migrate auth to zap RC

5dc31a6

Migrate tracing to zap RC

83cbdf2

Migrate service to zap RC

abf51f5

Migrate modules/rpc to zap RC

d19b759

Migrate modules/uhttp to zap RC

7afb03b

Migrate examples to zap RC

e4803b0

Akshay Shah added 2 commits February 23, 2017 16:19

Upgrade zap to v1.0.0-rc.2

653ac28

Again, this necessarily upgrades all other dependencies.

Fix service to use NamedError

68d4547

akshayjshah force-pushed the ajs-zap branch from 5b09b2b to 68d4547 Compare February 24, 2017 00:19

Simplify struct literals

80c2077

anuptalwalkar approved these changes Feb 24, 2017

View reviewed changes

anuptalwalkar merged commit 53d4fd6 into master Feb 24, 2017

anuptalwalkar deleted the ajs-zap branch February 24, 2017 00:57

Upgrade to zap 1.0 release candidate #290

Upgrade to zap 1.0 release candidate #290

Conversation

akshayjshah commented Feb 18, 2017 • edited Loading

No ulog.Log interface

No Sentry integration

No loggers in contexts

No log builder

Documentation

mention-bot commented Feb 18, 2017

akshayjshah commented Feb 18, 2017

ascandella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascandella commented Feb 18, 2017

akshayjshah commented Feb 19, 2017

ascandella commented Feb 19, 2017 via email

ascandella commented Feb 19, 2017

ascandella commented Feb 19, 2017

glibsm Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufdev commented Feb 20, 2017

akshayjshah commented Feb 20, 2017

akshayjshah commented Feb 23, 2017 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 23, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

akshayjshah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 23, 2017

coveralls commented Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuptalwalkar commented Feb 24, 2017

ascandella commented Feb 24, 2017

glibsm commented Feb 24, 2017

akshayjshah commented Feb 18, 2017 •

edited

Loading

glibsm Feb 19, 2017 •

edited

Loading

akshayjshah commented Feb 23, 2017 •

edited

Loading

ghost Feb 23, 2017 •

edited by ghost

Loading

coveralls commented Feb 24, 2017 •

edited

Loading