-
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
init jenkins input plugin #3292
Conversation
Hm... there is interesting mistake
|
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.
Not going to speak to the measurement name thing, but there are a few minor things I noticed looking over the code.
return fmt.Errorf("%s returned HTTP status %s", addr.String(), resp.Status) | ||
} | ||
|
||
parsed, err := parseResponse(resp.Body, addr.Host) |
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.
err
isn't checked.
plugins/inputs/jenkins/jenkins.go
Outdated
counters.tags["type"] = "counters" | ||
for k1, v1 := range jenkinsData.Counters { | ||
for k2, v2 := range v1 { | ||
counters.fields[fmt.Sprintf("%s.%s", k1, k2)] = v2 |
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 might be a nitpick, though since this gets called a lot it might be significant, but the fmt.Sprintf
adds overhead when you can do k1+"."+k2
. Same for the similar lines below.
uri := fmt.Sprintf("%s/metrics/%s/metrics", u.Url, u.Token) | ||
addr, err := url.Parse(uri) | ||
if err != nil { | ||
acc.AddError(fmt.Errorf("Unable to parse address '%s': %s", u, err)) |
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.
It looks like there should be a continue
here so that we skip the entry with the bad address.
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.
Thanks. Actually, I get that line of code from here (https://github.com/influxdata/telegraf/blob/master/plugins/inputs/nginx/nginx.go#L74) so you may want update nginx plugin 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.
Thanks for the heads up, I updated the nginx plugin b610276
@phemmer thanks for your review! |
It looks like Jenkins uses dropwizard style metrics, these are a little tricky to convert to a style that works well with Telegraf's data model. We have pull requests #2846 and #2369 which both attempt it. Ideally, we would complete the dropwizard json parser first and use it in this plugin, I was holding it up while I rewrote the configuration loading system but perhaps it is best to finish it up before hand so the plugins can use it. How about we discuss a general way to translate dropwizard metrics to Telegraf metrics over on #2846 and then we can use it here to parse the Jenkins metrics? |
Any progress on this work? |
should we close this in favor of #4022 ? |
Yes, #4022 will provide a plugin that doesn't require any Jenkins plugins. The Jenkins Metrics Plugin that this PR uses can now be monitored using the |
Initial PR. Plugin for getting metrics from Jenkins.
I'm not sure that I did all in right way, so I ask some questions here:
is a good idea to name measurement "jenkins" with a lot of fields like
or will be better to create a lot of measurements like
http.responseCodes.serverError
with fieldscount, m15_rate
, etc?