JMX Scraper: Kafka server, producer and consumer YAMLs and integration tests added #1670
Conversation
| @@ -86,12 +88,23 @@ static void afterAll() { | |||
|
|
|||
| @AfterEach | |||
| void afterEach() { | |||
| if (scraper != null && scraper.isRunning()) { | |||
There was a problem hiding this comment.
[for reviewer] I rearranged containers shutdown sequence to be in reverse order than starting to avoid unnecessary errors in the logs
| } | ||
|
|
||
| protected void verifyMetrics() { | ||
| MetricsVerifier metricsVerifier = createMetricsVerifier(); | ||
| await() | ||
| .atMost(Duration.ofSeconds(60)) | ||
| .pollInterval(Duration.ofSeconds(1)) |
There was a problem hiding this comment.
[for reviewer] Just to decrease CPU usage a bit if we do not have all metrics available immediately.
| .hasUnit("{request}") | ||
| .isGauge() | ||
| .hasDataPointsWithOneAttribute( | ||
| attributeWithAnyValue("client.id"))) // changed to follow semconv |
There was a problem hiding this comment.
[for reviewer] This is one of the changes to get closer to semconv standards
| metric | ||
| .hasDescription( | ||
| "The average number of bytes consumed for all topics per second") | ||
| .hasUnit("By") |
There was a problem hiding this comment.
[for reviewer] Changed from 'by' to follow semconv
| metric | ||
| .hasDescription( | ||
| "The average number of records consumed for all topics per second") | ||
| .hasUnit("{record}") |
There was a problem hiding this comment.
[for reviewer] Multiple units in this file have been changed from "1" to annotated form to follow semconv
| "sh", | ||
| "-c", | ||
| "echo 'Sending messages to test-topic-1'; " | ||
| + "i=1; while true; do echo \"Message $i\"; sleep .25; i=$((i+1)); done | /opt/bitnami/kafka/bin/kafka-console-producer.sh --bootstrap-server " |
There was a problem hiding this comment.
[for reviewer] Topic is automatically created when messages are sent to it - no need to manually create it
| AttributeMatcherGroup[] requestTypes = { | ||
| // attribute values are changed from lowercase to PascalCase | ||
| // because it is impossible to make them lowercase with YAML config | ||
| // TODO: What about const value attributes defined in YAML that are lowercase? |
There was a problem hiding this comment.
[for reviewer] We need to decide what to do with attribute names for Kafka.
Currently attributes extracted from bean names are in PascalCase, while the attributes defined by const(...) are lowercase (see YAMLs)
Maybe that is fine?
| .withEnv("JMX_PORT", Integer.toString(jmxPort)) | ||
| .withExposedPorts(jmxPort) | ||
| .waitingFor(Wait.forListeningPorts(jmxPort)); | ||
| // .waitingFor(Wait.forLogMessage(".*Sending messages to test-topic-1.*", 1)); |
There was a problem hiding this comment.
[for reviewer] Will be cleaned
| metric: kafka.purgatory.size | ||
| type: updowncounter | ||
| desc: The number of requests waiting in purgatory | ||
| unit: "{request}" |
There was a problem hiding this comment.
[for reviewer] Please verify all annotations of units. In the original files they were either in plular form, or were missing (unit: "1" was used instead)
| mapping: | ||
| fetch-rate: | ||
| desc: The number of fetch requests for all topics per second | ||
| unit: '{request}' |
There was a problem hiding this comment.
[for reviewer] Please verify all annotations of units. In the original files they were either in plular form, or were missing (unit: "1" was used instead)
| unit: ms | ||
| request-rate: | ||
| desc: The average number of requests sent per second | ||
| unit: '{request}' |
There was a problem hiding this comment.
[for reviewer] Please verify all annotations of units. In the original files they were either in plular form, or were missing (unit: "1" was used instead)
Three target systems connected to Kafka have been added:
To fully test Kafka metrics there is a need to spin multiple containers, so these three components can correctly interact and metrics can be correctly generated.
Base integration test class
TargetSystemIntegrationTestwas modified to support initialization of multiple containers.There are still some TODOs left that will be fixed in subsequent PRs
Part of #1362