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

Kafka module #2969

Merged
merged 1 commit into from Nov 16, 2016

Conversation

Projects
None yet
2 participants
@ruflin
Copy link
Collaborator

commented Nov 9, 2016

No description provided.

@ruflin ruflin force-pushed the ruflin:kafka-module branch from 9369ca5 to 3a674b0 Nov 9, 2016

@ruflin ruflin referenced this pull request Nov 10, 2016

Closed

Kafka consumer metricset #2977

@ruflin ruflin force-pushed the ruflin:kafka-module branch 3 times, most recently from b0e1154 to 89cabf1 Nov 10, 2016

@ruflin ruflin added review and removed in progress labels Nov 10, 2016

@ruflin ruflin force-pushed the ruflin:kafka-module branch from 89cabf1 to ef963bc Nov 10, 2016

type: list
description: >
List of replicas ids
- name: broker

This comment has been minimized.

Copy link
@urso

urso Nov 10, 2016

Collaborator

this field appears twice

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 11, 2016

Author Collaborator

removed

- name: brocker
type: integer
description: >
Broker id

This comment has been minimized.

Copy link
@urso

urso Nov 10, 2016

Collaborator

just publish broker ID or also the address? Sure, broker id is not supposed to change within cluster. But one might prefer to search by hostname.

These metadata are available from kafka: https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-MetadataResponse

The address can be found from Broker.Addr in sarma

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 11, 2016

Author Collaborator

Good idea. I also added the address

- name: replicas
type: list
description: >
List of replicas ids

This comment has been minimized.

Copy link
@urso

urso Nov 10, 2016

Collaborator

to make it more clear: set of alive nodes (broker IDs?)

You planning to integrate ISR notes? In kafka some slave nodes might not be in sync with broker yet.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 11, 2016

Author Collaborator

Changed it. I saw that there are some differences with notes and was thinking about adding a "status" field but not for the first version.

config.Net.ReadTimeout = m.Module().Config().Timeout
config.ClientID = "metricbeat"

client, err := sarama.NewClient([]string{m.Host()}, config)

This comment has been minimized.

Copy link
@urso

urso Nov 10, 2016

Collaborator

error check missing?

Why just one host. Often one configures a set of 'bootstrap' endpoints to query meta-data. If one fails, ask next endpoint.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 11, 2016

Author Collaborator

error check added.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 11, 2016

Author Collaborator

Having one host per metricset request is the usual pattern in Metricbeat. For simplicity reason I would not change this now for Kafka.

But this brings up an interesting question: Assuming each Kafka host is monitored with a list of Hosts, currently lots of data will be duplicated in elasticsearch as not only data is fetched from the partitions on the host the client connects to.

This comment has been minimized.

Copy link
@urso

urso Nov 15, 2016

Collaborator

In kafka (in comparison to other services), you do not monitor one host, but you monitor one cluster. That is, when configuring the hosts, all hosts MUST belong to the same cluster. Having multiple bootstraping hosts is for redundancy, to decrease the chances of errors.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 15, 2016

Author Collaborator

For the first version we should recommend to only use one Host as part of the docs but think long term on how we handle these kind of situations in metricbeat as this will not only apply to Kafka.

This comment has been minimized.

Copy link
@urso

urso Nov 15, 2016

Collaborator

Hmmm... is it required by module-interface to have a method doing Host() string? Why not have the module/metricset decide?

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 15, 2016

Author Collaborator

If the issue is only Kafka specific, we could also have a bootstrap_hosts: [...] config option for the Kafka module. Lets handle this as soon as it pops up as an issue.

for _, topic := range topics {
partitions, err := client.Partitions(topic)
if err != nil {
logp.Err("Fetch partition info for topic %s: %s", topic, err)

This comment has been minimized.

Copy link
@urso

urso Nov 10, 2016

Collaborator

is there a chance partitions not being empty in err? We still want to process potentially incomplete data?

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 11, 2016

Author Collaborator

Looking at the Sarama code, it looks like if an error is returned, partitions is always nil

This comment has been minimized.

Copy link
@urso

urso Nov 15, 2016

Collaborator

I still wonder if we want to return an error here. By returning an error, an error event will be published, right?

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 15, 2016

Author Collaborator

It will return an error event. But the problem is that it will abort the execution of the event fetch for all coming topics.

This comment has been minimized.

Copy link
@urso

urso Nov 15, 2016

Collaborator

In this case, we might consider adding an error event to events here.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 15, 2016

Author Collaborator

That would mean that we have an "error" entry at the wrong namespace of the event. Will open a separate issue to discuss this improvement for Metricbeat.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 15, 2016

Author Collaborator

Here is the related issue: #3004

replicas, err := client.Replicas(topic, partition)
if err != nil {
logp.Err("Fetch replicas for partition %s in topic %s: %s", partition, topic, err)
}

This comment has been minimized.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 11, 2016

Author Collaborator

I'm not even sure if we should provide the replicas ids here or not.

This comment has been minimized.

Copy link
@urso

urso Nov 15, 2016

Collaborator

I'd say yes, as these are part of the cluster state.

@ruflin ruflin force-pushed the ruflin:kafka-module branch from ef963bc to c406b3b Nov 11, 2016

@ruflin

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 11, 2016

@urso New version pushed.

@ruflin ruflin force-pushed the ruflin:kafka-module branch 3 times, most recently from ea68d8f to 9d667c0 Nov 11, 2016

@ruflin

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 14, 2016

jenkins, retest it

@ruflin ruflin force-pushed the ruflin:kafka-module branch from 9d667c0 to 332bb5f Nov 15, 2016

"id": brocker.ID(),
"address": brocker.Addr(),
},
}

This comment has been minimized.

Copy link
@urso

urso Nov 15, 2016

Collaborator

Hmm... I was in hope for getting the offset per replica and create an event per replicate. The broker is the current leader to a partition. All other brokers handling the same partition will be the replica. Every now and then the leader is re-elected, turning the broker into an replica and another former replica into the leader.

Some event like:

common.MapStr {
  "topic": topic,
  "partition": partition,
  "broker": common.MapStr {
    "id": id,
    "address": brokers[id],
  }
  "offset": offset,
  "leader": id == leaderID,
}

The address of every broker and broker id is included in meta-data.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 15, 2016

Author Collaborator

Follow up github issue: #3005

@ruflin ruflin force-pushed the ruflin:kafka-module branch from 7163451 to f637c7b Nov 15, 2016

Adding Kafka module to Metricbeat
First module added is partition which contains stats about each existing partition.

@ruflin ruflin force-pushed the ruflin:kafka-module branch from f637c7b to 8093e69 Nov 15, 2016

@urso urso merged commit 54ad3d2 into elastic:master Nov 16, 2016

4 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@ruflin ruflin referenced this pull request Nov 28, 2016

Closed

Additional Metricbeat Modules and Metricsets #1059

11 of 17 tasks complete

@monicasarbu monicasarbu deleted the ruflin:kafka-module branch Dec 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.