-
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
Dns query response time #694
Conversation
@@ -4,3 +4,5 @@ tivan | |||
.idea | |||
*~ | |||
*# | |||
|
|||
*.iml |
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.
please don't add this to the gitignore, these type of things should go in your global gitignore
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.
I saw .idea entry ;-)
But ok. I have removed this commit :-)
dnsQueryTime := float64(0) | ||
|
||
c := new(dns.Client) | ||
c.ReadTimeout = time.Duration(d.Timeout) * time.Second |
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.
this should be just d.Timeout * time.Second
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.
maybe not, nevermind
This looks great, a couple comments:
|
"recordType": d.RecordType, | ||
} | ||
|
||
acc.Add("dns", dnsQueryTime, tags) |
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.
change this to AddFields
, and add something like:
fields = map[string]string{"query_time_ms": dnsQueryTime}
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.
this is just in case we would like to add more metrics to this plugin in the future
I can rebase the branch up to master before merge if you wish |
@@ -157,6 +157,8 @@ Currently implemented sources: | |||
* bcache | |||
* couchdb | |||
* disque | |||
* dns |
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.
change to just * dns_query
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.
@sparrc package name too?
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.
yes, also the package (and directory) name
great, I'll get this merged once CI passes, thanks @mjasion |
Yeah, im working on it :P i will ping you |
64779d3
to
35b8313
Compare
small polishings added more record types - AAAA and ANY
- domains are optional - new record types
@sparrc CI passed :-) |
- renamed plugin to dns_query - domains are optional - new record types closes influxdata#694
Input plugin for measure dns response time