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

Add Kibana input plugin #4585

Merged
merged 4 commits into from Aug 24, 2018
Merged

Add Kibana input plugin #4585

merged 4 commits into from Aug 24, 2018

Conversation

lpic10
Copy link
Contributor

@lpic10 lpic10 commented Aug 22, 2018

Implements #4576

I tried to keep it simple and only collecting the metrics related to Kibana itself.

Required for all PRs:

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

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.

Looking good, any idea if we need to be concerned about elasticsearch version differences?

```toml
[[inputs.kibana]]
## specify a list of one or more Kibana servers
# you can add username and password to your url to use basic authentication:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a username and password option that will be shared among all servers. It also looks like this doesn't do basic authentication, we are just sending these in the userinfo section of the URL. Basic auth would be sending the data in the Authorization header https://tools.ietf.org/html/rfc7617.

I greatly prefer using basic auth instead, the userinfo section is deprecated: https://tools.ietf.org/html/rfc3986#section-3.2.1

- name (Kibana reported name)
- uuid (Kibana reported UUID)
- version (Kibana version)
- server (Kibana server hostname or IP)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the origin style as described in #4413


client *http.Client
catMasterResponseTokens []string
isMaster bool
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 we can remove isMaster and catMasterResponseTokens. It doesn't look like they are used.

type Kibana struct {
Local bool
Servers []string
HttpTimeout internal.Duration
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, HTTPTimeout or perhaps we just call it timeout.

return nil, err
}
tr := &http.Transport{
ResponseHeaderTimeout: k.HttpTimeout.Duration,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this timeout, its covered by the client timeout.

// NOTE: we are not going to read/discard r.Body under the assumption we'd prefer
// to let the underlying transport close the connection and re-establish a new one for
// future calls.
return "", fmt.Errorf("kibana: API responded with status-code %d, expected %d",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the transport should handle any errors for us with dead connections, can we remove this?

### Tags

- name (Kibana reported name)
- uuid (Kibana reported UUID)
Copy link
Contributor

Choose a reason for hiding this comment

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

UUID tags are always a little scary, when would this value change?

### Measurements & Fields

- kibana
- status: string (green, yellow, red)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do status as a tag? This would be our normal enum pattern as seen in http_response/net_response.

@lpic10
Copy link
Contributor Author

lpic10 commented Aug 24, 2018

@danielnelson thanks for the review, it should be better now.
I also added a new field requests_per_sec and fixed some issues with types.

any idea if we need to be concerned about elasticsearch version differences?

Yes, indeed this plugin will work on Kibana 6.x (tested from 6.0 to 6.4), but not on previous versions as the status API returns a different structure. For that I added a note in the doc, do you think that's enough?

@danielnelson danielnelson added this to the 1.8.0 milestone Aug 24, 2018
@danielnelson danielnelson merged commit 3d84cee into influxdata:master Aug 24, 2018
@danielnelson
Copy link
Contributor

That should be good, thanks again!

rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants