-
Notifications
You must be signed in to change notification settings - Fork 137
Adds emptiness time checking. #116
Adds emptiness time checking. #116
Conversation
@@ -39,6 +39,8 @@ type App struct { | |||
|
|||
tmplLock sync.Mutex | |||
tmpls map[string]*htmpl.Template | |||
|
|||
log bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be exported since you have no other way of setting this? I assume there would be cases where you'd want to disable the extra logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, def. agree on letting lib users decide to disable app logging, fixed on: chris-ramon@66b5c2e
c4976e7
to
66b5c2e
Compare
@@ -39,6 +39,8 @@ type App struct { | |||
|
|||
tmplLock sync.Mutex | |||
tmpls map[string]*htmpl.Template | |||
|
|||
Log bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to have this be a *log.Logger
instead. See how we do this for ChunkedCollector for example: https://github.com/sourcegraph/appdash/blob/master/collector.go#L148
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, def. agree on making it a logger
, fixed on 6f70b13.
One minor comment, aside from that LGTM |
66b5c2e
to
e5de73f
Compare
e5de73f
to
6f70b13
Compare
Perfect, thanks @chris-ramon ! :) |
Adds emptiness time checking.
Details
D3 Timeline
data. #108 which was already merged but Adds InfluxDB store. #99 PR included a commit that reverted it and includes feedback discussed here.