Metricbeat: Add configuration for RabbitMQ management prefix path #7074

jsoriano · 2018-05-11T07:43:09Z

Refactor RabbitMQ metricsets to reuse host parser and metricset builder
so management prefix path can be configured as an only module setting.
Check in tests the paths metricbeat is trying to access to collect the
metrics.

Previously it required to configure each metricset in its own module
block with the path in the host, as described in #6875.

houndci-bot · 2018-05-11T07:43:20Z

metricbeat/module/rabbitmq/url.go

+)
+
+var (
+	HostParser = parse.URLHostParserBuilder{


exported var HostParser should have comment or be unexported

houndci-bot · 2018-05-11T07:43:20Z

metricbeat/module/rabbitmq/url.go

+	defaultScheme = "http"
+	pathConfigKey = "management_path_prefix"
+
+	ConnectionsPath = "/api/connections"


exported const ConnectionsPath should have comment (or a comment on this block) or be unexported

houndci-bot · 2018-05-11T07:43:20Z

metricbeat/module/rabbitmq/metricset.go

+	*helper.HTTP
+}
+
+func NewMetricSet(base mb.BaseMetricSet, subPath string) (*MetricSet, error) {


exported function NewMetricSet should have comment or be unexported

houndci-bot · 2018-05-11T07:43:20Z

metricbeat/module/rabbitmq/metricset.go

+	"github.com/elastic/beats/metricbeat/mb"
+)
+
+type MetricSet struct {


exported type MetricSet should have comment or be unexported

houndci-bot · 2018-05-11T07:43:21Z

metricbeat/helper/http.go

@@ -103,6 +103,10 @@ func (h *HTTP) SetMethod(method string) {
 	h.method = method
 }

+func (h *HTTP) GetURI() string {


exported method HTTP.GetURI should have comment or be unexported

houndci-bot · 2018-05-11T07:59:29Z

metricbeat/module/rabbitmq/url.go

+
+	// Subpaths to management plugin endpoints
+	ConnectionsPath = "/api/connections"
+	ExchangesPath   = "/api/exchanges"


exported const ExchangesPath should have comment (or a comment on this block) or be unexported

houndci-bot · 2018-05-11T07:59:29Z

metricbeat/module/rabbitmq/url.go

+	defaultScheme = "http"
+	pathConfigKey = "management_path_prefix"
+
+	// Subpaths to management plugin endpoints


comment on exported const ConnectionsPath should be of the form "ConnectionsPath ..."

Refactor RabbitMQ metricsets to reuse host parser and metricset builder so management prefix path can be configured as an only module setting. Check in tests the paths metricbeat is trying to access to collect the metrics. Previously it required to configure each metricset in its own module block with the path in the host.

ruflin

This is a huge step forward in how we can simplify adding new modules. This will reduce the code in quite a few modules with multiple metricsets and will adding new ones much easier.

I was wondering if we should split out the code refactoring part from the management change? Not necessarily because of the management change but because I think the refactor deserves it's own commit to point people to to have a look.

ruflin · 2018-05-11T12:51:39Z

metricbeat/module/rabbitmq/connection/connection_test.go

@@ -14,13 +14,21 @@ import (
 )

 func TestFetchEventContents(t *testing.T) {
-	absPath, err := filepath.Abs("../_meta/testdata/")
+	absPath, _ := filepath.Abs("../_meta/testdata/")


I would add a assert.NoError( for these erors. This will make debugging test much easier.

ruflin · 2018-05-11T12:52:47Z

metricbeat/module/rabbitmq/exchange/exchange_test.go

+			w.Header().Set("Content-Type", "application/json;")
+			w.Write([]byte(response))
+		default:
+			w.WriteHeader(404)


Nice, I should steal this for my elasticsearch http tests.

ruflin · 2018-05-11T12:59:58Z

metricbeat/module/rabbitmq/exchange/exchange.go

 func init() {
-	if err := mb.Registry.AddMetricSet("rabbitmq", "exchange", New, hostParser); err != nil {


Did we miss that in our PR bonanza or this came in later?

ruflin · 2018-05-11T13:01:13Z

metricbeat/module/rabbitmq/url.go

+
+// Subpaths to management plugin endpoints
+const (
+	ConnectionsPath = "/api/connections"


I'm going forth an back if this should be in the module or in the metricset. Is there a benefit that the module knows about these paths?

ruflin · 2018-05-11T13:02:19Z

metricbeat/module/rabbitmq/url.go

+
+var (
+	// HostParser parses host urls for RabbitMQ management plugin
+	HostParser = parse.URLHostParserBuilder{


I wonder if it could become an issue that all Metricsets will now share one host parser as a global object?

It shouldn't be an issue.

ruflin · 2018-05-11T13:25:54Z

Merged it already to move forward with this. I really hope we get similar simplifications for other modules.

This is inspired by elastic#7074. More work is needed.

…astic#7074) Refactor RabbitMQ metricsets to reuse host parser and metricset builder so management prefix path can be configured as an only module setting. Check in tests the paths metricbeat is trying to access to collect the metrics. Previously it required to configure each metricset in its own module block with the path in the host.

jsoriano added in progress Pull request is currently in progress. module Metricbeat Metricbeat labels May 11, 2018

houndci-bot reviewed May 11, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-preffix-path branch from f2fbb5b to 40a3f2c Compare May 11, 2018 07:59

houndci-bot reviewed May 11, 2018

View reviewed changes

jsoriano force-pushed the rabbitmq-preffix-path branch 3 times, most recently from e04c9c9 to 1c66840 Compare May 11, 2018 08:14

jsoriano added review and removed in progress Pull request is currently in progress. labels May 11, 2018

jsoriano force-pushed the rabbitmq-preffix-path branch from 1c66840 to e91a07a Compare May 11, 2018 12:34

ruflin reviewed May 11, 2018

View reviewed changes

ruflin merged commit 2f3068a into elastic:master May 11, 2018

jsoriano mentioned this pull request May 14, 2018

RabbitMQ module path_prefix #6875

Closed

ruflin mentioned this pull request May 15, 2018

Move parts of metricset code to Elasticsearch module #7103

Merged

ruflin added a commit to ruflin/beats that referenced this pull request May 15, 2018

Move parts of metricset code to Elasticsearch module

f3f9e0b

This is inspired by elastic#7074. More work is needed.

jsoriano deleted the rabbitmq-preffix-path branch November 7, 2018 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metricbeat: Add configuration for RabbitMQ management prefix path #7074

Metricbeat: Add configuration for RabbitMQ management prefix path #7074

jsoriano commented May 11, 2018

houndci-bot May 11, 2018

houndci-bot May 11, 2018

houndci-bot May 11, 2018

houndci-bot May 11, 2018

houndci-bot May 11, 2018

houndci-bot May 11, 2018

houndci-bot May 11, 2018

ruflin left a comment

ruflin May 11, 2018

ruflin May 11, 2018

ruflin May 11, 2018

ruflin May 11, 2018

ruflin May 11, 2018

andrewkroh May 11, 2018

ruflin commented May 11, 2018

		func init() {
		if err := mb.Registry.AddMetricSet("rabbitmq", "exchange", New, hostParser); err != nil {

Metricbeat: Add configuration for RabbitMQ management prefix path #7074

Metricbeat: Add configuration for RabbitMQ management prefix path #7074

Conversation

jsoriano commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented May 11, 2018