-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement server stats and self-monitoring #1936
Conversation
|
588c8e7
to
3fbc155
Compare
log.Fatalf("failed to create retention policy for internal statistics: %s", err.Error()) | ||
} | ||
|
||
s.StartSelfMonitoring(database, policy, interval) |
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.
Missing an interval := config.Statistics.WriteInterval
?
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.
Very good -- I just fixed that on my local copy and see you spotted it. Fix coming.
There is 1 piece remaining for this, the ability to query a different server. This is being tracked by #1945 |
Green build. |
|
||
go func() { | ||
for { | ||
time.Sleep(interval) |
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.
Since the self-monitoring is a form of instrumentation, we should probably be using a time.Ticker.
For a good demonstration of the difference between the two, compare the output of using time.Sleep vs. using time.NewTicker.
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.
Ah, yes, I know about the tickers. I agree it would be nice to be precise with this stuff.
Updated to use |
@otoolep Would you mind rebasing this? If you think it's ready to go, I'll merge it in for RC12. |
Modeled on BoltDB and expvar stats.
This "name" becomes the Measurement name. In addition the implementation for "SHOW STATS" has been re-instated.
I have rebased this and the build is green. I have opened a ticket for the unrelated Travis failure. #1962 I consider this code ready to merge, though there is still significant more work to do in terms of the stats we need to collect. We also "lost" some stats due to the Raft refactor, since some stats are now only available at a lower-level than the This code has not been reviewed, so please review before merging. |
Awesome, this looks good to me. I'll give everyone else a chance to review and then I'll merge in a couple hours. |
Implement server stats and self-monitoring
No description provided.