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

Kapacitor input plugin #2031

Merged
merged 9 commits into from
Apr 27, 2017
Merged

Kapacitor input plugin #2031

merged 9 commits into from
Apr 27, 2017

Conversation

rossmcdonald
Copy link
Contributor

@rossmcdonald rossmcdonald commented Nov 10, 2016

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 Nov 14, 2016
@rossmcdonald
Copy link
Contributor Author

/cc @danielnelson

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Could also be good to add support for the normal TLS overrides, isn't a must for now though. https://github.com/influxdata/telegraf/blob/master/plugins/outputs/influxdb/influxdb.go#L31

func TestErrorHandling404(t *testing.T) {
badServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/endpoint" {
_, _ = w.Write([]byte(basicJSON))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why define all this JSON since we take the else branch?

// There were errors, so join them all together as one big error.
errorStrings := make([]string, 0, len(errorChannel))
for err := range errorChannel {
errorStrings = append(errorStrings, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to use acc.AddError(

#1446

@danielnelson danielnelson modified the milestones: 1.3.0, Future Milestone Mar 21, 2017
@nhaugo
Copy link
Contributor

nhaugo commented Mar 31, 2017

We need to add stats for:
Cardinality and errors

@nhaugo
Copy link
Contributor

nhaugo commented Apr 4, 2017

@danielnelson can you look at added these other two metrics?

@danielnelson danielnelson mentioned this pull request Apr 5, 2017
3 tasks
@danielnelson danielnelson force-pushed the ross-kapacitor-input branch 2 times, most recently from 9a864c4 to da5d8e1 Compare April 14, 2017 01:10
@danielnelson
Copy link
Contributor

@nathanielc Do you know where I can get the stats @nhaugo requested?

Also, can you check the metric renaming I did to be consistent with Telegraf's selfstats? There is one difference due to a bug in the internal plugin #2671

@phemmer
Copy link
Contributor

phemmer commented Apr 14, 2017

Just throwing in my own $0.02 because we've been bitten by this issue several times in the past with other plugins, but why not just pass through the fields directly, without unmarshaling onto a struct and then marshaling back out again? Unmarshaling onto a struct means that if kapacitor changes its metrics in any way, such as adding or removing, telegraf has to be updated. And if you want to use a different version of kapacitor than the one telegraf is coded for, then you're out of luck.

@nathanielc
Copy link
Contributor

@danielnelson Those stats will be added in v1.3 of Kapacitor, so to @phemmer's point we either need to change the plugin to be a pass through or update this plugin when they release.

Also I am willing to make the name change in Kapacitor to make it consistent if that helps.

Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

@danielnelson I am realizing I haven't ever reviewed this code, I took a quick look and added some comments. Sorry for the late review. Let me know how I can help move things forward.

for _, obj := range *s.Kapacitor {

// Strip out high-cardinality or duplicative tags
excludeTags := []string{"host", "cluster_id", "server_id"}
Copy link
Contributor

Choose a reason for hiding this comment

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

This struct me as odd, may this behavior can be triggered via config instead of assuming that this information is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about the comment, is it correct? We should, of course, make it hard to produce high-cardinality.

- num_tasks, integer
- kapacitor_edges
- collected, float
- emitted, float
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these stats are integers

- collected, float
- emitted, float
- kapacitor_ingress
- points_received, float
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an integer.

- query_errors, float
- tags_defaulted, float
- warns_triggered, float
- write_errors, float
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe everything in the kapacitor_nodes section is an integer.

@danielnelson
Copy link
Contributor

I'm reluctant to do passthrough in general because that would mean new fields just appear that are undocumented, and if any additional processing needs to be done on them we might get stuck by the need to preserve backwards compatibility.

I see some problems with using the current structs though, if the field does not exist then sending a zero value is not the right action. This might warrant doing an ad-hoc parsing, though I feel like unmarshal to struct is more easy to understand. I could turn all fields to pointers and add them only when not nil or maybe there is a better pattern that I can use?

@nathanielc On the topic of rename on the Kapacitor side, either way works for me but I don't think it is a requirement for Telegraf. I see you have stuck with the exact naming and capitalization that go uses, even though it is in contradiction with the rest of the endpoint...

@phemmer
Copy link
Contributor

phemmer commented Apr 14, 2017

I don't think it should be up to telegraf to document them. It should be up to the system which creates the metrics. If telegraf starts writing its own documentation on them, it might get it wrong. If we're just copy/pasting the upstream doc, why not just link to the upstream docs instead?

@danielnelson
Copy link
Contributor

I think telegraf just needs to document that they exist and may be sent, not the meaning of them. Otherwise you either need to run --test or read the source to know what the plugin will do.

@danielnelson danielnelson force-pushed the ross-kapacitor-input branch from da5d8e1 to bdd3d96 Compare April 27, 2017 00:32
@danielnelson danielnelson merged commit a3feacb into master Apr 27, 2017
@danielnelson danielnelson deleted the ross-kapacitor-input branch April 27, 2017 18:47
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
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.

6 participants