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

Automatically generate field names for HAProxy from columns in stats CSV #2193

Closed
wants to merge 9 commits into from

Conversation

G-regL
Copy link
Contributor

@G-regL G-regL commented Dec 21, 2016

Automatically generate field names for HAProxy from columns in stats CSV, instead of manually looping through to find metrics.

This will allow newly added metrics in the stats page to be sent through Telegraf without changing anything.

Is follow-up to #1179.

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)

@sparrc sparrc added this to the Future Milestone milestone Jan 9, 2017
Copy link
Contributor

@sparrc sparrc left a comment

Choose a reason for hiding this comment

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

can you explain what this feature adds? have you had problems with the current HAProxy plugin omitting metrics?

I can see that this is also a breaking change, can you explain what migrations people will have to perform to deal with the new data format?

@sparrc
Copy link
Contributor

sparrc commented Jan 25, 2017

Actually I'm just going to close this PR because I can see that it's still a breaking change. Unfortunately I don't think that this adds enough to justify breaking all current users of the plugin. If there are missing metrics then they will have to be added manually.

@sparrc sparrc closed this Jan 25, 2017
@G-regL
Copy link
Contributor Author

G-regL commented Jan 25, 2017

While I agree this in it's current form, this is a breaking change, but would you accept it if I returned the field names to match the existing format?

The primary reason for this PR is to expand on the list of metrics that this plugin collects, as those currently being gathered seemed to have been cherry-picked from the large number available in the stats pages.

The renaming of the metrics was done for simplicity and to stay in line with the vendors own stats names, but I'm not married to the idea and if keeping the existing name for metrics already being collected will mean this gets accepted then I'm fine with that. I'll just do a search/replace on the first row (field names) to keep them the same.

@sparrc
Copy link
Contributor

sparrc commented Jan 25, 2017

yes, in order to be accepted you need to:

  1. write a README explaining all metrics gathered
  2. document the aggregated metrics that you are counting up and explain what they are and why you are aggregating them
  3. keep existing metric names the same

@G-regL
Copy link
Contributor Author

G-regL commented Jan 27, 2017

Given the discussion in #2318 and #2323, what do you want to do with this PR? Do you plan to merge this one, or #2323? Or neither?

@sparrc
Copy link
Contributor

sparrc commented Jan 27, 2017

most likely #2323

@G-regL
Copy link
Contributor Author

G-regL commented Jan 30, 2017

Looking over it, I agree that it's a more comprehensive PR. I'll submit a separate PR for the aggregated server status counters from here once that one gets merged.

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.

2 participants