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

Wavefront output driver #3160

Merged
merged 3 commits into from
Sep 29, 2017
Merged

Wavefront output driver #3160

merged 3 commits into from
Sep 29, 2017

Conversation

puckpuck
Copy link
Contributor

@puckpuck puckpuck commented Aug 23, 2017

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This is a new output plugin for Wavefront. Wavefront is a SaaS for metrics. You can learn more about Wavefront on their website: https://www.wavefront.com

We have been using this output plugin for multiple months with a few Wavefront customers yielding great results, and are ready to release it to the community so it can be part of the main telegraf package.

This is a resubmit of this PR #2288 from a clean repo

@puckpuck
Copy link
Contributor Author

@danielnelson and @nhaugo If you need anything to test this so we can get it into 1.4, please let me know, and I will get it done.

@danielnelson
Copy link
Contributor

It will need to be a 1.5 feature.

@danielnelson danielnelson added this to the 1.5.0 milestone Aug 23, 2017
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017
red = 0.0

## Print additional debug information requires debug = true at the agent level
debug_all = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this and use the debug log level directly. In the future we will allow log level control on a per plugin basis in a uniform way. If you need to do something expensive related to the debug logging, you can get the loglevel using github.com/influxdata/wlog.

debug_all = false
```

Parameters:
Copy link
Contributor

Choose a reason for hiding this comment

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

This section should be removed since it duplicates much of the config file comments above. If there is something in particular that needs additional explanation (though I don't see anything like this) format it in a sub section like this example:

#### Convert Path

If the `convert_path` option is true any `_` in metric and field names will
be converted to the `metric_seperator`.


##

The Wavefront proxy interface can be simulated with this reader:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section should be removed, it isn't something a user of the plugin would need to worry about.

Most illegal characters in the metric name are automatically converted to `-`.
The `use_regex` setting can be used to ensure all illegal characters are properly handled, but can lead to performance degradation.

## Configuration:
Copy link
Contributor

Choose a reason for hiding this comment

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

Use three hashes: "### Configuration"

This plugin writes to a [Wavefront](https://www.wavefront.com) proxy, in Wavefront data format over TCP.


## Wavefront Data format
Copy link
Contributor

Choose a reason for hiding this comment

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

On this section use three hashes: ### Wavefront Data Format, but also move it below the Configuration section and combine with the "Allowed values for metrics". Some of this information you should consider removing since what is really important is how the Telegraf metrics will map onto the Wavefront metrics.

var retv string
switch p := v.(type) {
case bool:
if w.ConvertBool {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if !w.ConvertBool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the metric will get dropped

switch p := v.(type) {
case bool:
if w.ConvertBool {
if bool(p) {
Copy link
Contributor

Choose a reason for hiding this comment

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

p is already a bool at this point, same goes in the other cases.

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 have a new branch of code for this entire section which doesn't use strconv, and fixes the above as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will you update this PR then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, later today expect an update with fixes to address all your requested changes.


// err = w.Write(metrics)
// require.NoError(t, err)
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old code. Should of been removed.

index := 0
for k, v := range mTags {
if w.UseRegex {
tags[index] = fmt.Sprintf("%s=\"%s\"", sanitizedRegex.ReplaceAllString(k, "-"), tagValueReplacer.Replace(v))
Copy link
Contributor

Choose a reason for hiding this comment

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

Try sanitizedRegex.ReplaceAllLiteralString(k, "-"), I think it should be faster. Also, you can use a bytes.Buffer for writing the string which should be faster[1].

I'm not sure if the performance will be as good as using a replacer, but I think it can be made close enough that you don't need have both options.

I think ideally we would only have the regexp option, since it will be safer and we could remove a config option making plugin setup simpler.

[1] https://stackoverflow.com/questions/1760757/how-to-efficiently-concatenate-strings-in-go

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 did a simple test with ReplaceAllString, ReplaceAllLiteralString, and Replacer. Though ReplaceAllLiteralString does yield an 8% performance gain, it is still nowhere near Replacer which is 18x faster.

Code I used:

var testString = "this_is*my!test/string\\for replacement";

func BenchmarkReplaceAllString(b *testing.B) {
	for n := 0; n < b.N; n++ {
		sanitizedRegex.ReplaceAllString(testString, "-")
	}
}

func BenchmarkReplaceAllLiteralString(b *testing.B) {
	for n := 0; n < b.N; n++ {
		sanitizedRegex.ReplaceAllLiteralString(testString, "-")
	}
}

func BenchmarkReplacer(b *testing.B) {
	for n := 0; n < b.N; n++ {
		sanitizedChars.Replace(testString)
	}
}

Results from go bench

BenchmarkReplaceAllString	500000	      2463 ns/op
BenchmarkReplaceAllLiteralString	1000000	      2282 ns/op
BenchmarkReplacer	10000000	       124 ns/op

Copy link
Contributor

Choose a reason for hiding this comment

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

It might turn out that this is not very significant when compared to the rest of the code. What will happen if you send a metric with a non-sanitized character, will Wavefront return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Wavefront proxy will receive the data, then drop/block the point before sending it to the Wavefront service. No error will get returned to the agent, which is a big reason why we are putting lots of care into ensuring bad characters are not sent to the proxy.

name = sanitizedChars.Replace(name)
}

if w.ConvertPaths {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure about this option being a good idea, it will turn "foo.bar.idle_since" into "foo.bar.idle.since" which IMO is not very nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wavefront uses '.' delimiters in our metrics browser, and things like measurement: cpu field: usage_idle should become 'cpu.usage.idle' for proper browsing. This provides a better UX in our product. This gets especially important in application level metrics where you can return dozens of fields many with the same field prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes the metrics that Telegraf gathers use an underscore only because the name has a space in it, they don't always indicate a hierarchical relationship.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We understand that, but Wavefront has made the conscience decision that hierarchies will appear more often than spaces/special chars where an underscore will appear. We had a few meetings about this, took samples from various input plugins, and decided that converting to dots (default) is the more likely scenario for our customers to have the best possible experience with the product.

@puckpuck
Copy link
Contributor Author

I updated this PR to incorporate all the changes your requested. The failed circleci test is from another input plugin. @danielnelson anything I can do to get around another plugin causing the failed test?

@danielnelson
Copy link
Contributor

Sorry about that, there is not really anything you can do, hopefully I will be able to make the tests more reliable in the future.

@danielnelson danielnelson merged commit 366f3f5 into influxdata:master Sep 29, 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
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants