Add kafka common fields at the module level #7767

jsoriano · 2018-07-26T18:46:07Z

Move some common fields in kafka metricsets to the module level as proposed by @gingerwizard. This will allow to correlate documents of different events.

houndci-bot · 2018-07-26T18:46:14Z

metricbeat/module/kafka/consumergroup/consumergroup.go

@@ -95,25 +97,36 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
 	}, nil
 }

-func (m *MetricSet) Fetch() ([]common.MapStr, error) {
+func (m *MetricSet) Fetch(r mb.ReporterV2) {


exported method MetricSet.Fetch should have comment or be unexported

jsoriano · 2018-07-26T18:46:46Z

Marked in progress because the integration test added for retrieving consumergroup data is not working yet 🤔

ruflin · 2018-07-27T06:36:37Z

metricbeat/module/kafka/consumergroup/_meta/fields.yml

@@ -26,10 +27,12 @@

    - name: topic
      type: keyword
+      deprecated: true


Kafka is still beta, so we could remove the old entries. Instead we could have a more detailed explanation in the changelog or the breaking changes.

Even being in beta, I'd prefer to keep the old fields in case someone is using them, we can remove them later, I add a comment about this in the changelog.

ruflin · 2018-07-27T06:37:06Z

++ on the shared fields

jsoriano · 2018-07-27T12:38:50Z

metricbeat/module/kafka/consumergroup/_meta/data.json

            "broker": {
-                "address": "kafka0:9092",
+                "address": "172.18.0.2:9092",


Interesting, in my laptop the broker address is different for consumergroup and partition metricsets, this is not good for correlations 🤔

Using advertised address in both metricsets now, so it can be more easily used for correlations.

Interesting, in my laptop the broker address is different for consumergroup and partition metricsets, this is not good for correlations

This is correct behavior. In kafka a broker can fulfill a many tasks. E.g. being the leader of a partition producers write to, or manage a consumer group. Consumer groups can be handled by very different brokers. Yet, the consumer group index used to store committed offsets from consumers is another internal "topic" potentially owned by yet another broker. State is very localized.

Yes, the problem was that in one metricset we were using the address we use to connect, and on the other the advertised address, so the broker was the same, but different addresses were being collected.

jsoriano · 2018-07-30T10:22:31Z

metricbeat/module/kafka/consumergroup/consumergroup_integration_test.go

+
+	ms := mbtest.NewReportingMetricSetV2(t, getConfig())
+	for retries := 0; retries < 3; retries++ {
+		err = mbtest.WriteEventsReporterV2(ms, t, "")


Added retries here because it seems that consumer group can take its time to appear in metrics.

jsoriano · 2018-07-30T10:36:55Z

metricbeat/module/kafka/consumergroup/consumergroup_integration_test.go

+func startConsumer(t *testing.T, topic string) (io.Closer, error) {
+	brokers := []string{getTestKafkaHost()}
+	topics := []string{topic}
+	return saramacluster.NewConsumer(brokers, "test-group", topics, nil)


I started trying to create and joining a consumer group using sarama only, but I ended copying parts of sarama-cluster, so even if it is only for tests I think it worths to add this library.

Number of consumers in a consumer-group are limited by the number of partitions in a topic. If we pre-initialize the topics with a known set of partitions, then we just have to setup N go-routines joining a consumer group. Using saramacluster might be the easier option though :)

ruflin

Going to merge as changelog can still be cleaned up later.

ruflin · 2018-08-02T07:23:28Z

CHANGELOG.asciidoc

@@ -83,6 +84,8 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
 *Metricbeat*
 - Redis `info` `replication.master_offset` has been deprecated in favor of `replication.master.offset`.{pull}7695[7695]



Is this newline on purpose?

@gingerwizard

Move some common fields in kafka metricsets to the module level as proposed by @gingerwizard. This will allow to correlate documents of different events. (cherry picked from commit 4fedeec)

@gingerwizard

Move some common fields in kafka metricsets to the module level as proposed by @gingerwizard. This will allow to correlate documents of different events. (cherry picked from commit 4fedeec)

Kafka dashboards calculate cardinalities of composed fields, this is expensive in query time, but can be optimized if the composed fields are precalculated. Continues with elastic#7767, needed for elastic#8457.

Kafka dashboards calculate cardinalities of composed fields, this is expensive in query time, but can be optimized if the composed fields are precalculated. Continues with #7767, needed for #8457.

Kafka dashboards calculate cardinalities of composed fields, this is expensive in query time, but can be optimized if the composed fields are precalculated. Continues with elastic#7767, needed for elastic#8457. (cherry picked from commit 78ef120)

Kafka dashboards calculate cardinalities of composed fields, this is expensive in query time, but can be optimized if the composed fields are precalculated. Continues with #7767, needed for #8457. (cherry picked from commit 78ef120)

Metricbeat: Move common kafka fields to the module level

c2a07c1

jsoriano added in progress Pull request is currently in progress. module Metricbeat Metricbeat labels Jul 26, 2018

houndci-bot reviewed Jul 26, 2018

View reviewed changes

Add kafka consumergroup integration test

f93a014

jsoriano force-pushed the kafka-common-fields branch from a1307ce to f93a014 Compare July 26, 2018 18:47

ruflin reviewed Jul 27, 2018

View reviewed changes

jsoriano mentioned this pull request Jul 27, 2018

Add Dashboards for Metricbeat #5451

Closed

23 tasks

jsoriano added 4 commits July 27, 2018 11:36

Added comment

b16057d

Extended description of change in changelog

730c2a3

Consumer group created, still no metrics

eb87b1f

Use sarama-cluster to create the consumer

a367375

jsoriano commented Jul 27, 2018

View reviewed changes

jsoriano added 4 commits July 27, 2018 14:43

Fix notice for sarama-cluster

f87c08e

Used advertised address for kafka.broker.address

f823976

Merge remote-tracking branch 'origin/master' into kafka-common-fields

1ac6cf7

Set deprecation versions

27e3f1d

jsoriano commented Jul 30, 2018

View reviewed changes

jsoriano requested review from urso and gingerwizard July 30, 2018 11:03

jsoriano added review and removed in progress Pull request is currently in progress. labels Jul 30, 2018

jsoriano added 3 commits July 30, 2018 16:54

Fix changelog

7ca1bc8

Merge remote-tracking branch 'origin/master' into kafka-common-fields

5cb5d04

Merge remote-tracking branch 'origin/master' into kafka-common-fields

1a78eb0

urso approved these changes Jul 31, 2018

View reviewed changes

jsoriano added the needs_backport PR is waiting to be backported to other branches. label Aug 1, 2018

ruflin approved these changes Aug 2, 2018

View reviewed changes

ruflin merged commit 4fedeec into elastic:master Aug 2, 2018

jsoriano mentioned this pull request Aug 9, 2018

Cherry-pick #7767 to 6.x: Add kafka common fields at the module level #7930

Merged

jsoriano added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Aug 9, 2018

jsoriano mentioned this pull request Aug 20, 2018

Metricbeat Kafka Module Enhancements #3005

Closed

5 tasks

jsoriano mentioned this pull request Oct 1, 2018

Add kafka fields needed for dashboards #8504

Merged

jsoriano mentioned this pull request Oct 18, 2018

Cherry-pick #8504 to 6.x: Add kafka fields needed for dashboards #8644

Merged

jsoriano mentioned this pull request Nov 28, 2018

metricbeat: fetch offset info from primary partiton instead of replica partition #5918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kafka common fields at the module level #7767

Add kafka common fields at the module level #7767

jsoriano commented Jul 26, 2018

houndci-bot Jul 26, 2018

jsoriano commented Jul 26, 2018

ruflin Jul 27, 2018

jsoriano Jul 27, 2018

ruflin commented Jul 27, 2018

jsoriano Jul 27, 2018

jsoriano Jul 30, 2018

urso Jul 31, 2018

jsoriano Jul 31, 2018

jsoriano Jul 30, 2018

jsoriano Jul 30, 2018

urso Jul 31, 2018

ruflin left a comment

ruflin Aug 2, 2018

		@@ -83,6 +84,8 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
		Metricbeat
		- Redis `info` `replication.master_offset` has been deprecated in favor of `replication.master.offset`.{pull}7695[7695]

Add kafka common fields at the module level #7767

Add kafka common fields at the module level #7767

Conversation

jsoriano commented Jul 26, 2018

Choose a reason for hiding this comment

jsoriano commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Jul 27, 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