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

Update Kafka JMX Script #216

Merged
merged 14 commits into from
Jan 31, 2022
Merged

Update Kafka JMX Script #216

merged 14 commits into from
Jan 31, 2022

Conversation

dehaansa
Copy link
Contributor

@dehaansa dehaansa commented Jan 25, 2022

Description: Update Kafka JMX metrics to update names, units, fix counters, and get more metrics.

Testing: Updated Integration test

@@ -28,6 +30,29 @@
import org.testcontainers.utility.MountableFile;

abstract class KafkaIntegrationTest extends AbstractIntegrationTest {
Startable createTopics =
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for moving this? Previously the containers were listed in order of execution which was a bit nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I had moved it initially when I was trying to leverage it for another container but ended up removing that and didn't move it back. Will resolve.

@@ -112,8 +112,8 @@ void beforeEach() {
otlpServer.reset();
}

@SafeVarargs
protected final void waitAndAssertMetrics(Consumer<Metric>... assertions) {
@SuppressWarnings("varargs")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think I see a varargs usage in this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, fixed.

@rmfitzpatrick
Copy link
Contributor

rmfitzpatrick commented Jan 27, 2022

@dehaansa I'll be investigating the test mbean server issues shortly.

edit: may provide more context and earlier failures: #218

@dehaansa
Copy link
Contributor Author

@dehaansa I'll be investigating the test mbean server issues shortly.

edit: may provide more context and earlier failures: #218

Merged in the changes, looks like everything is passing, can I get a re-review @rmfitzpatrick ?

Copy link
Contributor

@rmfitzpatrick rmfitzpatrick left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements!

@rmfitzpatrick rmfitzpatrick merged commit eef8c17 into open-telemetry:main Jan 31, 2022
@dehaansa dehaansa deleted the kafka-jmx-updates branch January 31, 2022 18:46
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