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.

Sign up for GitHub

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 kafka fields needed for dashboards #8504

Merged
merged 6 commits into from
Oct 18, 2018
Merged

Add kafka fields needed for dashboards #8504

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5a0a8b8
Add kafka fields needed for dashboards
jsoriano Oct 1, 2018
c8c4845
Changes in comments
jsoriano Oct 1, 2018
0a6334e
Merge remote-tracking branch 'origin/master' into kafka-id-fields
jsoriano Oct 16, 2018
b266c2a
Typo on fields type
jsoriano Oct 16, 2018
41c151f
Use replica broker id for composed partition broker id
jsoriano Oct 17, 2018
935a103
Don't publish the partition broker id on the consumergroups
jsoriano Oct 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
- Support for Kafka 2.0.0 {pull}8399[8399]
- Add container image for docker metricsets. {issue}8214[8214] {pull}8438[8438]
- Add support for `full` status page output for php-fpm module as a separate metricset called `process`. {pull}8394[8394]
- Precalculate composed id fields for kafka dashboards {pull}8504[8504]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Dot missing at the end.


*Packetbeat*

Expand Down
18 changes: 18 additions & 0 deletions metricbeat/docs/fields.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7118,6 +7118,24 @@ type: long
Partition id.


--

*`kafka.partition.topic_id`*::
+
--
type: keyworkd

Unique id of the partition in the topic.

--

*`kafka.partition.topic_broker_id`*::
+
--
type: keyworkd

Unique id of the partition in the topic and the broker.

--

[float]
Expand Down
10 changes: 10 additions & 0 deletions metricbeat/module/kafka/_meta/fields.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,13 @@
type: long
description: >
Partition id.

- name: partition.topic_id
type: keyworkd
description:
Unique id of the partition in the topic.

- name: partition.topic_broker_id
type: keyworkd
description:
Unique id of the partition in the topic and the broker.
6 changes: 4 additions & 2 deletions metricbeat/module/kafka/consumergroup/_meta/data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"client": {
"host": "172.18.0.1",
"id": "sarama",
"member_id": "sarama-714cfb8b-39e5-4128-9109-e5056c5e8f56"
"member_id": "sarama-fcb5a5db-0474-4f3a-a5af-29e2f14549c5"
},
"error": {
"code": 0
Expand All @@ -29,7 +29,9 @@
"topic": "metricbeat-test"
},
"partition": {
"id": 0
"id": 0,
"topic_broker_id": "0-metricbeat-test-0",
"topic_id": "0-metricbeat-test"
},
"topic": {
"name": "metricbeat-test"
Expand Down
9 changes: 8 additions & 1 deletion metricbeat/module/kafka/consumergroup/consumergroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package consumergroup

import (
"crypto/tls"
"fmt"

"github.com/pkg/errors"

Expand Down Expand Up @@ -111,6 +112,10 @@ func (m *MetricSet) Fetch(r mb.ReporterV2) {
}

emitEvent := func(event common.MapStr) {
// Helpful IDs for dashboards
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather describe it as helpful for queries.

For the future: I wonder if this is work that should be done in the processors to have it config based instead of in the code.

partitionTopicID := fmt.Sprintf("%d-%s", event["partition"], event["topic"])
partitionTopicBrokerID := fmt.Sprintf("%s-%d", partitionTopicID, m.broker.ID())

// TODO (deprecation): Remove fields from MetricSetFields moved to ModuleFields
event["broker"] = brokerInfo
r.Event(mb.Event{
Expand All @@ -120,7 +125,9 @@ func (m *MetricSet) Fetch(r mb.ReporterV2) {
"name": event["topic"],
},
"partition": common.MapStr{
"id": event["partition"],
"id": event["partition"],
"topic_id": partitionTopicID,
"topic_broker_id": partitionTopicBrokerID,
},
},
MetricSetFields: event,
Expand Down
2 changes: 1 addition & 1 deletion metricbeat/module/kafka/fields.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions metricbeat/module/kafka/partition/_meta/data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"address": "172.18.0.2:9092",
"id": 0
},
"id": 0,
"offset": {
"newest": 0,
"oldest": 0
Expand All @@ -26,11 +27,13 @@
"replica": 0
},
"topic": {
"name": "foo-1532945633-185372965"
}
"name": "foo-1538389014-739473801"
},
"topic_broker_id": "0-foo-1538389014-739473801-0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not topic.id and topic.broker_id?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not really sure about the naming and placing of these composed ids, in demo they are directly at the module level as kafka.topic_partition_broker_id, I moved them to partition as in some way they identify the partition. For the same reason I'd keep them as fields directly under the partition, and I wouldn't expect other fields under kafka.partition.topic. kafka.partition.topic.id may be seen also as if it should be the same as kafka.topic.id.

In any case I don't have a strong opinion on these names, so as you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now that topic.id shows up in different events. So kafka.topic.id would probably make most sense? Meaning that we should move out topic from partition to also have kafka.topic.name (would be a breaking change)?

Copy link
Member Author

Choose a reason for hiding this comment

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

kafka.topic.name was added in #7767 to replace kafka.partition.topic.name and kafka.consumergroup.topic.name as a common name. These fields contain the topic name as seen by kafka, old ones are marked as deprecated.

The fields added in this PR are created by us to help on queries and don't exist in kafka, they identify the partition in the topic and in the broker (there can be two partitions with the same id, but of different topics, and two partitions with the same id and different topics and/or brokers), this is why I'd add them under kafka.partition, I see them more like identifiers of the partition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that that more then one topic.id shows up in a single event?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure of following you... a single event will have an only kafka.topic.id and an only kafka.partition.topic_id (or however we call it), and both ids will be different, being the second one composed by the first one and kafka.partition.id.

Copy link
Contributor

Choose a reason for hiding this comment

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

That actually answers my questions. There are 2 topic.id in 1 event so there have to be different fields like you suggested.

"topic_id": "0-foo-1538389014-739473801"
},
"topic": {
"name": "foo-1532945633-185372965"
"name": "foo-1538389014-739473801"
}
},
"metricset": {
Expand Down
13 changes: 10 additions & 3 deletions metricbeat/module/kafka/partition/partition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package partition
import (
"crypto/tls"
"errors"
"fmt"

"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/common/cfgwarn"
Expand Down Expand Up @@ -167,8 +168,17 @@ func (m *MetricSet) Fetch(r mb.ReporterV2) {
}
}

// Helpful IDs for dashboards
partitionTopicID := fmt.Sprintf("%d-%s", partition.ID, topic.Name)
partitionTopicBrokerID := fmt.Sprintf("%s-%d", partitionTopicID, b.ID())

// create event
event := common.MapStr{
// Common `kafka.partition` fields
"id": partition.ID,
"topic_id": partitionTopicID,
"topic_broker_id": partitionTopicBrokerID,

"topic": evtTopic,
"broker": evtBroker,
"partition": partitionEvent,
Expand All @@ -183,9 +193,6 @@ func (m *MetricSet) Fetch(r mb.ReporterV2) {
ModuleFields: common.MapStr{
"broker": evtBroker,
"topic": evtTopic,
"partition": common.MapStr{
"id": partition.ID,
},
},
MetricSetFields: event,
})
Expand Down