-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 parser #4402
Wavefront parser #4402
Conversation
Haven't had time to review yet, but the Have you thought at all about the format of this field? Would it be only a fqdn or would it potentially have things such as a port number or schema? |
yeah that's what the source tag represents in Wavefront. Where the data came from. This used to be called host, but now we see data from serverless stuff, some odd funky monitoring things, sometimes it's even the name of your car (yes no lie), so host isn't really appropriate anymore. In Wavefront source/host is a required tag, since it becomes part of the main index in the TSDB. Wether it's a short name or fqdn Wavefront doesn't care. It's just a string representing an entity. We prefer to let the user/customer select which one they want to use, though most seem to lean towards short names, however some larger enterprises want an fqdn |
If we started using a standard name it would just be convention only in the same way the host tag is now. I would be happy with
Obviously this is a bikeshed issue to a large degree but I'm having a hard time deciding if the tag needs to be unique to identify the source or if it would make sense to only specify the system it is on, maybe all of the examples could just be
|
I'm not sure what to do with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, approving since only changes are minor formatting
if s, ok := mTags["source"]; ok { | ||
source = s | ||
delete(mTags, "source") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: no empty line here
plugins/parsers/wavefront/parser.go
Outdated
import ( | ||
"bufio" | ||
"bytes" | ||
"github.com/influxdata/telegraf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move non-stdlib imports below, separated by an empty line
package wavefront | ||
|
||
import ( | ||
"github.com/influxdata/telegraf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, move non-stdlib imports below
made minor formatting changes as requested. Also added docs. Realized just now it should be documented. Parser has no config so the docs are pretty simple. |
@puckpuck, I don't see the new Wavefront parser show up in the release notes (other than this issue being listed) or on the main Telegraf readme (parsers). Did it actually get released with 1.8? |
@randallt it is part of 1.8. It did make it to the Features section of the release notes, but not the Parsers section. @danielnelson ? |
The main README.md also needs to be updated:
|
I'll create a PR for the main readme. The input data format in docs is good though. |
Come to think of it, since the docs for parsers/serializers were split all the links on the main readme need to be updated as well. |
Sorry about that, I'll get it fixed. |
Required for all PRs:
Parser for Wavefront Data Format
Does require a small fix to Wavefront output plugin in order to anticipate the presence of a
source
tag which will overrule any otherhost
,hostname
, etc. tag. Source/Host is a special class tag in Wavefront.Note: No readme.md was created, since other parsers don't have one.