-
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
Add ActiveMQ input plugin #2689
Conversation
plugins/inputs/activemq/README.md
Outdated
$ ./telegraf -config telegraf.conf -input-filter activemq -test | ||
queues_metrics,name=sandra,host=88284b2fe51b consumer_count=0i,enqueue_count=0i,dequeue_count=0i,size=0i 1492610703000000000 | ||
queues_metrics,name=Test,host=88284b2fe51b dequeue_count=0i,size=0i,consumer_count=0i,enqueue_count=0i 1492610703000000000 | ||
topics_metrics,name=ActiveMQ.Advisory.MasterBroker\ ,host=88284b2fe51b size=0i,consumer_count=0i,enqueue_count=1i,dequeue_count=0i 1492610703000000000 |
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.
Seems like there might be a bug where the name has trailing whitespace characters.
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.
Don't forget to regenerate this section
plugins/inputs/activemq/activemq.go
Outdated
## Required password used for HTTP Basic Authentication | ||
# password = "admin" | ||
## Required ActiveMQ webadmin root path | ||
# webadmin = "admin" |
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.
Do a two space indent here
plugins/inputs/activemq/activemq.go
Outdated
a.GatherSubscribersMetrics(acc, subscribers) | ||
|
||
return nil | ||
} |
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 reduce the amount of vertical whitespace in this function?
plugins/inputs/activemq/activemq.go
Outdated
defer resp.Body.Close() | ||
|
||
return ioutil.ReadAll(resp.Body) | ||
} |
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 reduce the amount of vertical whitespace in this function?
remove trailing spaces for queue name in gather metrics remove indent
fix for review influxdata#2689
@mlabouardy I think the tests will pass if we rebase the branch, could you do that for me and then I'll take a final look. |
remove trailing spaces for queue name in gather metrics remove indent
@danielnelson done |
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.
Here are a few minor things we should address before merging:
plugins/inputs/activemq/activemq.go
Outdated
} | ||
|
||
func (a *ActiveMQ) GetMetrics(keyword string) ([]byte, error) { | ||
client := &http.Client{} |
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.
To avoid any issues sharing the underlying http.Transport
we don't use the DefaultTransport. We can use the same pattern as the apache plugin:
Add a field to the ActiveMQ struct to hold the client, and initialize it the first call to Gather
:
telegraf/plugins/inputs/apache/apache.go
Lines 69 to 75 in 0759c8b
if n.client == nil { | |
client, err := n.createHttpClient() | |
if err != nil { | |
return err | |
} | |
n.client = client | |
} |
Initialization:
telegraf/plugins/inputs/apache/apache.go
Lines 95 to 109 in 0759c8b
func (n *Apache) createHttpClient() (*http.Client, error) { | |
tlsCfg, err := n.ClientConfig.TLSConfig() | |
if err != nil { | |
return nil, err | |
} | |
client := &http.Client{ | |
Transport: &http.Transport{ | |
TLSClientConfig: tlsCfg, | |
}, | |
Timeout: n.ResponseTimeout.Duration, | |
} | |
return client, nil | |
} |
plugins/inputs/activemq/activemq.go
Outdated
records["enqueue_count"] = queue.Stats.EnqueueCount | ||
records["dequeue_count"] = queue.Stats.DequeueCount | ||
|
||
acc.AddFields("queues_metrics", records, 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.
To avoid confusion with other plugins we should call this activemq_queues
.
plugins/inputs/activemq/activemq.go
Outdated
records["enqueue_count"] = topic.Stats.EnqueueCount | ||
records["dequeue_count"] = topic.Stats.DequeueCount | ||
|
||
acc.AddFields("topics_metrics", records, 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.
Rename activemq_topics
plugins/inputs/activemq/activemq.go
Outdated
records["enqueue_counter"] = subscriber.Stats.EnqueueCounter | ||
records["dequeue_counter"] = subscriber.Stats.DequeueCounter | ||
|
||
acc.AddFields("subscribers_metrics", records, 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.
Rename activemq_subscribers
- enqueue_counter | ||
- dequeue_counter | ||
|
||
### 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.
We will also need a tag for the server for differentiation when running the plugin multiple times. Following the discussion on #4413, let's add source
and port
tags to all three measurements:
source=192.168.50.10,port=8161
plugins/inputs/activemq/README.md
Outdated
$ ./telegraf -config telegraf.conf -input-filter activemq -test | ||
queues_metrics,name=sandra,host=88284b2fe51b consumer_count=0i,enqueue_count=0i,dequeue_count=0i,size=0i 1492610703000000000 | ||
queues_metrics,name=Test,host=88284b2fe51b dequeue_count=0i,size=0i,consumer_count=0i,enqueue_count=0i 1492610703000000000 | ||
topics_metrics,name=ActiveMQ.Advisory.MasterBroker\ ,host=88284b2fe51b size=0i,consumer_count=0i,enqueue_count=1i,dequeue_count=0i 1492610703000000000 |
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.
Don't forget to regenerate this section
@danielnelson done |
@danielnelson I cannot make the test pass, any idea what I did wrong ? |
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 think these are the cause of test failures:
plugins/inputs/activemq/activemq.go
Outdated
|
||
tags["name"] = topic.Name | ||
tags["source"] = a.Server | ||
tags["port"] = string(a.Port) |
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 won't work, instead use strconv.FormatInt
.
|
||
var acc testutil.Accumulator | ||
|
||
activeMQ := new(ActiveMQ) |
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.
You will need to initialize the struct in the same way as in the init function:
activeMQ := &ActiveMQ{
Server: "localhost",
Port: 8161,
}
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.
done
@danielnelson resolved |
plugins/inputs/activemq/activemq.go
Outdated
var sampleConfig = ` | ||
## Required ActiveMQ Endpoint | ||
# server = "192.168.50.10" | ||
## Required ActiveMQ port |
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/suggestion: add newlines between config options
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.
done
@danielnelson when this can be merged ? :) |
Resolves #2622
Required for all PRs: