Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement telegraf collecting stats on itself #2043

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Implement telegraf collecting stats on itself #2043

merged 1 commit into from
Dec 5, 2016

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Nov 14, 2016 •
edited
Loading

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

closes #1348

@sparrc sparrc force-pushed the cs1348-selfstat branch 21 times, most recently from 9f9c0f7 to e2a26cd Compare November 16, 2016 13:22
ghost
ghost reviewed Nov 17, 2016
View reviewed changes
selfstat/README.md
@@ -0,0 +1 @@
# TODO
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be awesome to include

@ghost
Copy link

ghost commented Nov 17, 2016

It's quite a long diff, is this commit fixing a few things and also adding new functionality? I don't know the culture of this project (sorry), but it seems this would be an easier review if chunked a bit.

@sparrc sparrc force-pushed the cs1348-selfstat branch 3 times, most recently from 6f93d85 to fdf1a60 Compare November 18, 2016 16:16
@sparrc
Copy link
Contributor Author

sparrc commented Nov 18, 2016

thanks @rud-bookbites, but I'm actually the maintainer of the project, you don't need to worry about reviewing this ;-)

@sparrc sparrc force-pushed the cs1348-selfstat branch 2 times, most recently from 7bbdf36 to 7abf7c4 Compare December 2, 2016 12:29
@sparrc sparrc force-pushed the cs1348-selfstat branch 5 times, most recently from 35e8112 to a4a269e Compare December 2, 2016 15:21
dgnorton
dgnorton reviewed Dec 5, 2016
View reviewed changes
Copy link
Contributor

@dgnorton dgnorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about done flag and same question applies to several other spots in the PR with removal of done and Quiet.

internal/models/makemetric.go
@@ -123,11 +122,9 @@ func makemetric(
case float64:
// NaNs are invalid values in influxdb, skip measurement
if math.IsNaN(val) || math.IsInf(val, 0) {
if debug {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it going to spam the log without the debug flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been caught in an earlier change. The debug flag has been removed in favor of the D! log prefix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was caught. I commented on the old code. Just making sure this was the intended behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, this is the intended behavior

@sparrc sparrc merged commit d71a42c into master Dec 5, 2016
@sparrc sparrc deleted the cs1348-selfstat branch December 7, 2016 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgnorton dgnorton dgnorton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Telegraf should send stats about itself
2 participants
@sparrc @dgnorton