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

Add integration/system tests for Kafka JMX metricsets #14677

Merged
merged 15 commits into from
Nov 25, 2019

Conversation

ChrsMark
Copy link
Member

This PR adds integration and system tests for Kafka JMX metricsets as a follow-up of #14330

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added module needs_backport PR is waiting to be backported to other branches. :Testing Team:Integrations Label for the Integrations team v7.6.0 labels Nov 21, 2019
@ChrsMark ChrsMark requested a review from a team as a code owner November 21, 2019 08:35
@ChrsMark ChrsMark self-assigned this Nov 21, 2019
@ChrsMark ChrsMark added [zube]: In Progress in progress Pull request is currently in progress. labels Nov 21, 2019
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added [zube]: In Review review and removed [zube]: In Progress in progress Pull request is currently in progress. labels Nov 21, 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>
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.

I think we should better follow conventions for some field names.

Proposed changes in integration tests would be nice to have, but not required.

type: float
- name: topic.messages_in
description: The incoming message rate per topic
type: float
- name: net.bytes_in
Copy link
Member

Choose a reason for hiding this comment

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

Following field conventions this should probably be:

Suggested change
- name: net.bytes_in
- name: net.in.bytes_per_sec

Copy link
Member Author

Choose a reason for hiding this comment

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

@sorantis is this ok from product perspective or we should strictly follow the naming provided in the initial issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

there's no strict requirement on the naming. The field names were taken from the ER attached to the original issue. Do we have the mentioned naming convention somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

- name: net.bytes_in
description: The incoming byte rate
type: float
- name: net.bytes_out
Copy link
Member

Choose a reason for hiding this comment

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

And:

Suggested change
- name: net.bytes_out
- name: net.out.bytes_per_sec

- name: net.bytes_out
description: The outgoing byte rate
type: float
- name: net.bytes_rejected
Copy link
Member

Choose a reason for hiding this comment

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

And:

Suggested change
- name: net.bytes_rejected
- name: net.rejected.bytes_per_sec

metricbeat/module/kafka/broker/broker_integration_test.go Outdated Show resolved Hide resolved
metricbeat/module/kafka/broker/broker_integration_test.go Outdated Show resolved Hide resolved
metricbeat/tests/system/test_kafka.py Show resolved Hide resolved
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

@jsoriano thanks for the suggestions! Changed the fields' naming since @sorantis went along and implemented the tests using the NewFetcher.

@ChrsMark ChrsMark merged commit a5d86cb into elastic:master Nov 25, 2019
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Nov 26, 2019
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Nov 26, 2019
ChrsMark added a commit that referenced this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants