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

[#1262] Add Graylog input plugin #1261

Closed
wants to merge 1 commit into from
Closed

[#1262] Add Graylog input plugin #1261

wants to merge 1 commit into from

Conversation

alimousazy
Copy link
Contributor

@alimousazy alimousazy commented May 25, 2016

Required for all PRs:

  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

@alimousazy alimousazy changed the title Add Graylog input plugin [#1261] Add Graylog input plugin May 25, 2016
@alimousazy alimousazy changed the title [#1261] Add Graylog input plugin [#1262] Add Graylog input plugin May 25, 2016
## HTTP Header parameters (all values must be strings)
[inputs.graylog.headers]
Authorization = "Basic YWRtaW46YWRtaW4"
Content-Type = "application/json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why allow users to set these headers? Content-Type and Accept should ALWAYS be application/json. Authorization should be controlled by providing an "auth_key" argument to users, which the plugin would then internally create the Authorization header for the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@alimousazy
Copy link
Contributor Author

All comments done, Please verify @sparrc

"jvm.memory.pools.Metaspace.committed"
]
## User name and password
[inputs.graylog.auth_key]
Copy link
Contributor

Choose a reason for hiding this comment

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

don't make inputs.graylog.auth_key a map, just use two arguments username and password directly in the plugin

@sparrc
Copy link
Contributor

sparrc commented May 27, 2016

@alimousazy Looks much better, thanks for the revisions, but there is one general change that I think still needs to happen. Since graylog appears to be returning a specific JSON format, like:

{
        "full_name": "org.graylog2.shared.journal.KafkaJournal.writeTime",
        "metric": {
          "time": {
            "min": 99
          },
          "rate": {
            "total": 10,
            "mean": 2
          },
          "duration_unit": "microseconds",
          "rate_unit": "events/second"
        },
        "name": "writeTime",
        "type": "hdrtimer"
      }

It seems like you should marshal the response JSON onto an object that looks something like this:

type ResponseMetrics struct {
  total   int
  Metrics []map[string]metric `json:"metrics"`
}

type metric struct {
  FullName string `json:"full_name"`
  name string
  type string
  metric map[string]interface{}
}

then you can make metric.FullName the telegraf metric name. Flatten metric.metric and make those the fields, and set metric.name and metric.type as tags.


import (
"bytes"
b64 "encoding/base64"
Copy link
Contributor

Choose a reason for hiding this comment

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

don't rename this import, use the built-in name

@alimousazy
Copy link
Contributor Author

@sparrc Done
1- Writing comments about metrics.
2- Replace auth key with username and password.
3- Write Specific parser for Graylog metrics and change the structure

type GrayLog struct {
Name string
Servers []string
TagKeys []string
Copy link
Contributor

Choose a reason for hiding this comment

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

remove TagKeys from the code since they're not used anymore

@alimousazy
Copy link
Contributor Author

@sparrc

  • Change the comment (less than 80 char) done.
  • Tag Key removed.
  • Added support for Namespace end point (Grouping for metrics) if User doesn't want to provide list of metrics to pull.

}

type GrayLog struct {
Name string
Copy link
Contributor

Choose a reason for hiding this comment

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

Name doesn't appear to be getting used anymore, can you also remove it?

@sparrc
Copy link
Contributor

sparrc commented Jun 1, 2016

Looking good @alimousazy, only other thing is Graylog.Name doesn't appear to be used anymore

add unit test for graylog
@alimousazy
Copy link
Contributor Author

Done removing name @sparrc

@sparrc sparrc closed this in f08a27b Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants