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.

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

Adding Varnish HTTP Cache input plugin #1173

Merged
merged 2 commits into from
May 24, 2016

Conversation

robinpercy-xm
Copy link
Contributor

collects metrics from the varnishstat command

@sparrc sparrc mentioned this pull request May 18, 2016
## By default, telegraf gather stats for 3 metric points.
## Setting stats will override the defaults (["%s"]).
## stats may also be set to ["all"], which will collect all stats
stats = ["%[1]s"]
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used formatted string to avoid repeating the defaultStats list in multiple places. I'll clean it up and update the README and CHANGELOG as per your comments on my other PRs.

cmd := exec.Command(cmdName, cmdArgs...)
var out bytes.Buffer
cmd.Stdout = &out
err := cmd.Run()
Copy link
Contributor

Choose a reason for hiding this comment

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

- Linked to varnish in input README
- Updated README/CHANGELOG
- Cleaned up sampleConfig to remove formatting
- Shorted lines to 80 chars (except where test input requires long strings)
- Using internal.RunTimeout to wrap call to varnishtat
- Added dummy file for windows
@sparrc
Copy link
Contributor

sparrc commented May 23, 2016

what would you think about the first part of the measurement being lower-cased & integrated into the measurement name?

ie, instead of

varnish
  - MAIN.vsm_overflowed
  - MGT.uptime

it could be

varnish_main
  - vsm_overflowed
varnish_mgt
  - uptime

@sparrc
Copy link
Contributor

sparrc commented May 23, 2016

although what you have is the format that varnish gives it in....so it might be best to keep it as-is

@robinpercy-xm
Copy link
Contributor Author

Yeah, I'm inclined to leave them as they are. As you said, it mirrors what varnishstat provides, which makes query building intuitive. The InfluxDB docs seem to recommend tags over measurement names for organizing. Though, the stated advantages don't really apply to this scenario, since the field names are all unique across sections anyway. Series cardinality stays the same in either case. Are there any other considerations around transmission or storage that could tip the scales?

@sparrc sparrc merged commit 1248934 into influxdata:master May 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants