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

[Metricbeat] Add Kafka JMX metricsets #14330

Merged
merged 33 commits into from
Nov 20, 2019

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Oct 30, 2019

Implements kafka.broker, kafka.consumer, kafka.producer metricset of #13366.

Depends on:

Manual Testing

Broker metricset

In order to prepare the proper environment to test these metricsets, the quickest way is to reuse the Docker env of the module. This can be achieved by first of all running the integration tests of the module which will take care of bringing up the container with the proper configuration.

To run the tests:
go test -v -tags=integration github.com/elastic/beats/metricbeat/module/kafka/partition

Verify that tests are successful and you can go on to manual testing. After the tests have finished the container is still alive so we can use it for the manual testing too.

Check what are the exported ports from Docker with a docker ps:

5da9631cfa25        docker.elastic.co/observability-ci/beats-integration-kafka:2.1.1-1   "/run.sh"           23 minutes ago      Up 23 minutes (healthy)   2181/tcp, 0.0.0.0:32771->8774/tcp, 0.0.0.0:32770->8775/tcp, 0.0.0.0:32769->8779/tcp, 0.0.0.0:32768->9092/tcp   metricbeat_kafka_1

In this example the mapping is:

  • broker port: 0.0.0.0:32769->8779
  • producer port: 0.0.0.0:32770->8775
  • consumer port: 0.0.0.0:32771->8774

So we configure the metricsets' hosts accordingly.

Enable the Kafka module and use the following configuration in modules.d/kafka.yml:

# Metrics collected from a Kafka broker using Jolokia
- module: kafka
  metricsets:
    - broker
  period: 10s
  hosts: ["localhost:32769"]

# Metrics collected from a Java Kafka consumer using Jolokia
- module: kafka
  metricsets:
    - consumer
  period: 10s
  hosts: ["localhost:32771"]

# Metrics collected from a Java Kafka producer using Jolokia
- module: kafka
  metricsets:
    - producer
  period: 10s
  hosts: ["localhost:32770"]

Run Metricbeat and verify that metrics are being collected from all of the 3 metricsets we configured.

When done remove the testing container.

@ChrsMark ChrsMark added in progress Pull request is currently in progress. module Metricbeat Metricbeat [zube]: In Progress Team:Integrations Label for the Integrations team v7.6.0 labels Oct 30, 2019
@ChrsMark ChrsMark requested a review from a team as a code owner October 30, 2019 14:37
@ChrsMark ChrsMark self-assigned this Oct 30, 2019
@ChrsMark ChrsMark changed the title Add kafka broker jmx Add kafka jmx metrics Oct 31, 2019
@ChrsMark ChrsMark changed the title Add kafka jmx metrics [Metricbeat] Add Kafka JMX metricsets Nov 1, 2019
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark force-pushed the add_kafka_broker_jmx branch from 7194cc1 to c2da3a3 Compare November 12, 2019 09:12
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added the needs_backport PR is waiting to be backported to other branches. label Nov 15, 2019
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

This is looking good, I am requesting only some small changes in Dockerfile and configuration.

@@ -8,20 +8,30 @@ ENV KAFKA_LOGS_DIR="/kafka-logs"
ENV _JAVA_OPTIONS "-Djava.net.preferIPv4Stack=true"
ENV TERM=linux

RUN apt-get update && apt-get install -y curl openjdk-8-jre-headless netcat dnsutils
RUN apt-get update && apt-get install -y curl openjdk-8-jre-headless netcat dnsutils wget
Copy link
Member

Choose a reason for hiding this comment

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

curl was already installed, you can use it, and you can also use retry as is being used to download kafka.


RUN mkdir -p ${KAFKA_LOGS_DIR} && mkdir -p ${KAFKA_HOME} && \
curl -J -L -s -f -o - https://github.com/kadwanev/retry/releases/download/1.0.1/retry-1.0.1.tar.gz | tar xfz - -C /usr/local/bin && \
retry --min 1 --max 180 -- curl -J -L -s -f --show-error -o $INSTALL_DIR/kafka.tgz \
"https://archive.apache.org/dist/kafka/${KAFKA_VERSION}/kafka_2.11-${KAFKA_VERSION}.tgz" && \
tar xzf ${INSTALL_DIR}/kafka.tgz -C ${KAFKA_HOME} --strip-components 1

RUN wget -O /opt/jolokia-jvm-1.5.0-agent.jar http://search.maven.org/remotecontent\?filepath\=org/jolokia/jolokia-jvm/1.5.0/jolokia-jvm-1.5.0-agent.jar
# RUN mv jolokia-jvm-1.5.0-agent.jar /opt/jolokia-jvm-1.5.0-agent.jar
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out code.

@@ -1,5 +1,5 @@
- module: kafka
metricsets: ["consumergroup", "partition"]
metricsets: ["broker", "consumer", "consumergroup", "partition", "producer"]
period: 10s
hosts: ["localhost:9092"]
enabled: true
Copy link
Member

@jsoriano jsoriano Nov 15, 2019

Choose a reason for hiding this comment

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

As the hosts in the config should be different for the different metricsets, we could put it in different blocks in the reference configuration, also adding a note about the expected host, something like what we do in the kubernetes module, something like this:

# Kafka metrics collected using the Kafka protocol
- module: kafka
  enabled: true
  metricsets:
    - consumergroup
    - partition
  period: 10s
  hosts: ["localhost:9092"]

# Metrics collected from a Kafka broker using Jolokia
#- module: kafka
#  enabled: true
#  metricsets: ['broker']
#  period: 10s
#  hosts: ['localhost:8779']

# Metrics collected from a Java Kafka consumer using Jolokia
#- module: kafka
#  enabled: true
#  metricsets: ['consumer']
#  period: 10s
#  hosts: ['localhost:8775']

# Metrics collected from a Java Kafka producer using Jolokia
#- module: kafka
#  enabled: true
#  metricsets: ['producer']
#  period: 10s
#  hosts: ['localhost:8774']

# - consumergroup
# - producer
period: 10s
hosts: ["localhost:9092"]
Copy link
Member

Choose a reason for hiding this comment

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

We may need a similar separation as the one suggested above for the reference config. Or we could have just one config.yml with the suggestions mentioned before and remove the config.reference.yml.

KAFKA_OPTS="-Djava.security.auth.login.config=/kafka/bin/jaas-kafka-client-consumer.conf -javaagent:/opt/jolokia-jvm-1.5.0-agent.jar=port=8774,host=0.0.0.0" \
${KAFKA_HOME}/bin/kafka-console-consumer.sh --topic=test --bootstrap-server=localhost:9091 --consumer.config ${KAFKA_HOME}/bin/sasl-producer.properties > /dev/null &

wait_for_port 8774
Copy link
Member

Choose a reason for hiding this comment

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

Note to myself 🙂 consider separating it in several entrypoints for #14087.
Don't do it here because it will only complicate this PR and it is not needed by now.


[float]
=== Compatibility
The module has been tested with Kafka 2.1.1. Other versions are expected to work.
Copy link
Member

Choose a reason for hiding this comment

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

We may add here also a link to the Jolokia module for the compatibility notes there.

description: Broker metrics from Kafka Broker JMX
release: beta
fields:
- name: mbean
Copy link
Member

Choose a reason for hiding this comment

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

mbeans should be always in the same field at the root level (something like jmx.mbean).

This is probably something to change in the Jolokia module, so we can keep this here by now.

@@ -0,0 +1,20 @@
This metricset periodically fetches JMX metrics from Kafka Producers JMX.
Copy link
Member

Choose a reason for hiding this comment

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

We may want to explicitly mention that this only works for consumers implemented in Java, and same thing in the producer metricset.

Btw:

Suggested change
This metricset periodically fetches JMX metrics from Kafka Producers JMX.
This metricset periodically fetches JMX metrics from Kafka Consumers JMX.


[float]
=== Usage
The Producer metricset requires <<metricbeat-module-jolokia,Jolokia>>to fetch JMX metrics. Refer to the link for instructions about how to use Jolokia.
Copy link
Member

Choose a reason for hiding this comment

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

We should also mention something about the metricsets requiring Jolokia in the module documentation (metricbeat/module/kafka/_meta/docs.asciidoc).

@ChrsMark ChrsMark force-pushed the add_kafka_broker_jmx branch from baba2b9 to 651fe2f Compare November 15, 2019 14:21
@ChrsMark ChrsMark added the test-plan Add this PR to be manual test plan label Nov 20, 2019
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

Merging this one since Travis succeeded and Jenkins failure is irrelevant.

Tests will be added in a follow up PR.

@ChrsMark ChrsMark merged commit 87c49ac into elastic:master Nov 20, 2019
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Nov 20, 2019
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Nov 20, 2019
@andresrc andresrc assigned sayden and unassigned ChrsMark Jan 14, 2020
@sayden sayden added the test-plan-regression Manually testing this PR found a regression label Jan 30, 2020
@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 6, 2020

Regression should be fixed now with the fixes at #15957.

@sayden
Copy link
Contributor

sayden commented Feb 6, 2020

Tested manually, good work!

@sayden sayden added test-plan-ok This PR passed manual testing and removed test-plan-regression Manually testing this PR found a regression labels Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Pull request is currently in progress. Metricbeat Metricbeat module Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan test-plan-ok This PR passed manual testing v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants