FrankChen021 commented on code in PR #20559:
URL: https://github.com/apache/druid/pull/20559#discussion_r4316047315
##########
extensions-contrib/kafka-emitter/src/test/java/org/apache/druid/emitter/kafka/KafkaEmitterTest.java:
##########
@@ -673,4 +696,462 @@ public String getFeed()
return "testFeed";
}
}
+
+ // ==================== Sampling Tests ====================
+
+ @Test
+ public void testShouldSampleOut_noSamplingWhenRateIs100()
+ {
+ final KafkaEmitter emitter = initKafkaEmitter(newSamplingConfig(100,
ImmutableSet.of("query/time"), null));
+ ServiceMetricEvent event =
ServiceMetricEvent.builder().setMetric("query/time", 100).build("svc", "host");
+ Assertions.assertFalse(emitter.shouldSampleOut(event));
+ }
+
+ @Test
+ public void testShouldSampleOut_noSamplingWhenSampledMetricsEmpty()
+ {
+ final KafkaEmitter emitter = initKafkaEmitter(newSamplingConfig(100, null,
null));
Review Comment:
in this case, I think the rate should not be defined as 100, otherwise we
don't knonw if it's because rate 100 allows no sample or the empty metric
allows it
##########
docs/development/extensions-contrib/kafka-emitter.md:
##########
@@ -49,6 +49,9 @@ All the configuration parameters for the Kafka emitter are
under `druid.emitter.
| `druid.emitter.kafka.extra.dimensions` | Optional JSON configuration to
specify a map of extra string dimensions for the events emitted. These can help
make groups in your monitoring environment. | no | none |
| `druid.emitter.kafka.producer.hiddenProperties` | JSON configuration to
specify sensitive Kafka producer properties such as username and password.
This property accepts a
[DynamicConfigProvider](../../operations/dynamic-config-provider.md)
implementation. | no | none |
| `druid.emitter.kafka.producer.shutdownTimeout` | Duration in milliseconds
the Kafka producer waits for pending requests to finish before shutting down. |
no | Long.MAX_VALUE |
+| `druid.emitter.kafka.samplingRate` | Percentage (0-100) of matching metric
events to keep. `100` keeps every event; `0` drops every matching event. See
[Metric sampling](#metric-sampling). | no | 100 |
Review Comment:
please clarifiy that if this one is integer or decimal
##########
docs/development/extensions-contrib/kafka-emitter.md:
##########
@@ -49,6 +49,9 @@ All the configuration parameters for the Kafka emitter are
under `druid.emitter.
| `druid.emitter.kafka.extra.dimensions` | Optional JSON configuration to
specify a map of extra string dimensions for the events emitted. These can help
make groups in your monitoring environment. | no | none |
| `druid.emitter.kafka.producer.hiddenProperties` | JSON configuration to
specify sensitive Kafka producer properties such as username and password.
This property accepts a
[DynamicConfigProvider](../../operations/dynamic-config-provider.md)
implementation. | no | none |
| `druid.emitter.kafka.producer.shutdownTimeout` | Duration in milliseconds
the Kafka producer waits for pending requests to finish before shutting down. |
no | Long.MAX_VALUE |
+| `druid.emitter.kafka.samplingRate` | Percentage (0-100) of matching metric
events to keep. `100` keeps every event; `0` drops every matching event. See
[Metric sampling](#metric-sampling). | no | 100 |
+| `druid.emitter.kafka.sampledMetrics` | JSON array of metric names to sample,
such as `["query/time"]`. Names must match exactly. If empty, sampling applies
to all metrics. | no | `[]` |
+| `druid.emitter.kafka.sampledNodeTypes` | JSON array of services to sample,
such as `["druid/broker"]`. If empty, sampling applies to all services. | no |
`[]` |
Review Comment:
let's use `sampledServiceTypes` , because in the source code of metric
event, the type uses the name `service`.
##########
extensions-contrib/kafka-emitter/src/test/java/org/apache/druid/emitter/kafka/KafkaEmitterTest.java:
##########
@@ -673,4 +696,462 @@ public String getFeed()
return "testFeed";
}
}
+
+ // ==================== Sampling Tests ====================
+
+ @Test
+ public void testShouldSampleOut_noSamplingWhenRateIs100()
+ {
+ final KafkaEmitter emitter = initKafkaEmitter(newSamplingConfig(100,
ImmutableSet.of("query/time"), null));
+ ServiceMetricEvent event =
ServiceMetricEvent.builder().setMetric("query/time", 100).build("svc", "host");
+ Assertions.assertFalse(emitter.shouldSampleOut(event));
+ }
+
+ @Test
+ public void testShouldSampleOut_noSamplingWhenSampledMetricsEmpty()
+ {
+ final KafkaEmitter emitter = initKafkaEmitter(newSamplingConfig(100, null,
null));
+ ServiceMetricEvent event =
ServiceMetricEvent.builder().setMetric("query/time", 100).build("svc", "host");
+ Assertions.assertFalse(emitter.shouldSampleOut(event));
+ }
+
+ /**
+ * samplingRate=10, both sampledMetrics and sampledNodeTypes empty → samples
ALL metrics at 10%.
+ * With rate=0 we can deterministically assert true.
+ */
+ @Test
+ public void testShouldSampleOut_samplesAllWhenRateSetButFiltersEmpty()
+ {
+ final KafkaEmitter emitter = initKafkaEmitter(newSamplingConfig(0, null,
null));
Review Comment:
comment says the rate is 10, but 0 here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
