abhishekrb19 commented on code in PR #14935:
URL: https://github.com/apache/druid/pull/14935#discussion_r1409802424
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -47,6 +46,8 @@ All the configuration parameters for the Prometheus emitter
are under `druid.emi
| `druid.emitter.prometheus.pushGatewayAddress` | Pushgateway address.
Required if using `pushgateway` strategy.
| no | none
|
| `druid.emitter.prometheus.flushPeriod` | Emit metrics to Pushgateway
every `flushPeriod` seconds. Required if `pushgateway` strategy is used.
| no | 15
|
| `druid.emitter.prometheus.extraLabels` | JSON key-value pairs for
additional labels on all metrics. Keys (label names) must match the regex
`[a-zA-Z_:][a-zA-Z0-9_:]*`. Example: `{"cluster_name": "druid_cluster1", "env":
"staging"}`. | no | none
|
+| `druid.emitter.prometheus.pushGatewayDeleteOnShutdown` | Flag to delete
metrics from Pushgateway on shutdown. Works only if `pushgateway` strategy is
used.
| no | false |
+| `druid.emitter.prometheus.waitForShutdownDelay` | Time to wait for
Pushgateway to delete metrics on shutdown in seconds (e.g. 60). Works only if
`pushgateway` strategy is used. Be aware, that task can terminate before the
deletion is performed, when the [Peon's
`druid.indexer.task.gracefulShutdownTimeout` is
used](https://druid.apache.org/docs/latest/configuration/#additional-peon-configuration).
| no | none |
Review Comment:
A few suggestions, including the duration recorded in milliseconds instead
of seconds:
```suggestion
| `druid.emitter.prometheus.waitForShutdownDelay` | Time in
milliseconds to wait for peon tasks to delete metrics from the Pushgateway on
shutdown (e.g. 60_000). Applicable only when `pushgateway` strategy is used and
`pushGatewayDeleteOnShutdown` is set to true. Be aware that there's no
guarantee that a peon task will delete metrics from the gateway if the
configured delay is more than the [Peon's
`druid.indexer.task.gracefulShutdownTimeout`](https://druid.apache.org/docs/latest/configuration/#additional-peon-configuration).
| no | none |
```
##########
extensions-contrib/prometheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/PrometheusEmitterTest.java:
##########
@@ -333,7 +341,61 @@ public void testEmitterConfig()
true,
true,
60,
+ null,
+ false,
null
);
}
+
+ @Test
+ public void testEmitterWithDeleteOnShutdown() throws IOException
+ {
+ PrometheusEmitterConfig emitterConfig = new
PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.pushgateway,
"namespace3", null, 0, "pushgateway", true, true, 60, null, true, null);
+
+ PushGateway mockPushGateway = mock(PushGateway.class);
+ mockPushGateway.push(anyObject(CollectorRegistry.class), anyString(),
anyObject(ImmutableMap.class));
+ expectLastCall().atLeastOnce();
+ mockPushGateway.delete(anyString(), anyObject(ImmutableMap.class));
+ expectLastCall().atLeastOnce();
+
+ EasyMock.replay(mockPushGateway);
+
+ PrometheusEmitter emitter = new PrometheusEmitter(emitterConfig);
+ emitter.start();
+ emitter.setPushGateway(mockPushGateway);
+ ServiceMetricEvent build = ServiceMetricEvent.builder()
+ .setDimension("task",
"index_parallel")
+ .setMetric("task/run/time",
500)
+
.build(ImmutableMap.of("service", "peon", "host", "druid.test.cn"));
+ emitter.emit(build);
+ emitter.flush();
+ emitter.close();
+
+ EasyMock.verify(mockPushGateway);
+ }
+
+ @Test
+ public void testEmitterWithoutDeleteOnShutdown() throws IOException
+ {
+ PrometheusEmitterConfig emitterConfig = new
PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.pushgateway,
"namespace3", null, 0, "pushgateway", true, true, 60, null, false, null);
Review Comment:
Can we also add a test with a positive value for `waitForShutdownDelay`
(1_000) and verify that the delete happens once?
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java:
##########
@@ -202,11 +209,35 @@ public void close()
{
if (strategy.equals(PrometheusEmitterConfig.Strategy.exporter)) {
if (server != null) {
- server.stop();
+ server.close();
}
} else {
exec.shutdownNow();
flush();
+
+ try {
+ if (config.getWaitForShutdownDelay() > 0) {
+ Thread.sleep(config.getWaitForShutdownDelay());
+ }
+ }
+ catch (InterruptedException e) {
+ log.error(e, "Interrupted while waiting for shutdown delay. Deleting
metrics now.");
Review Comment:
```suggestion
log.error(e, "Interrupted while waiting for shutdown delay. Deleting
metrics from the push gateway now.");
```
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -47,6 +46,8 @@ All the configuration parameters for the Prometheus emitter
are under `druid.emi
| `druid.emitter.prometheus.pushGatewayAddress` | Pushgateway address.
Required if using `pushgateway` strategy.
| no | none
|
| `druid.emitter.prometheus.flushPeriod` | Emit metrics to Pushgateway
every `flushPeriod` seconds. Required if `pushgateway` strategy is used.
| no | 15
|
| `druid.emitter.prometheus.extraLabels` | JSON key-value pairs for
additional labels on all metrics. Keys (label names) must match the regex
`[a-zA-Z_:][a-zA-Z0-9_:]*`. Example: `{"cluster_name": "druid_cluster1", "env":
"staging"}`. | no | none
|
+| `druid.emitter.prometheus.pushGatewayDeleteOnShutdown` | Flag to delete
metrics from Pushgateway on shutdown. Works only if `pushgateway` strategy is
used.
| no | false |
Review Comment:
I think `deletePushGatewayMetricsOnShutdown` is clearer, although a bit
verbose:
```suggestion
| `druid.emitter.prometheus.deletePushGatewayMetricsOnShutdown ` | Flag to
delete metrics from Pushgateway on task shutdown. Works only if `pushgateway`
strategy is used.
| no | false |
```
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java:
##########
@@ -202,11 +209,35 @@ public void close()
{
if (strategy.equals(PrometheusEmitterConfig.Strategy.exporter)) {
if (server != null) {
- server.stop();
+ server.close();
}
} else {
exec.shutdownNow();
flush();
+
+ try {
+ if (config.getWaitForShutdownDelay() > 0) {
+ Thread.sleep(config.getWaitForShutdownDelay());
+ }
+ }
+ catch (InterruptedException e) {
+ log.error(e, "Interrupted while waiting for shutdown delay. Deleting
metrics now.");
+ }
+ finally {
+ deletePushGatewayMetrics();
+ }
+ }
+ }
+
+ private void deletePushGatewayMetrics()
+ {
+ if (pushGateway != null && config.isPushGatewayDeleteOnShutdown()) {
+ try {
+ pushGateway.delete(config.getNamespace(),
ImmutableMap.of(config.getNamespace(), identifier));
+ }
+ catch (IOException e) {
+ log.error(e, "Unable to delete prometheus metrics from pushGateway");
Review Comment:
```suggestion
log.error(e, "Unable to delete prometheus metrics from push
gateway");
```
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -103,6 +111,8 @@ public PrometheusEmitterConfig(
this.addHostAsLabel = addHostAsLabel;
this.addServiceAsLabel = addServiceAsLabel;
this.extraLabels = extraLabels != null ? extraLabels :
Collections.emptyMap();
+ this.pushGatewayDeleteOnShutdown = pushGatewayDeleteOnShutdown != null &&
pushGatewayDeleteOnShutdown;
+ this.waitForShutdownDelay = waitForShutdownDelay != null ?
waitForShutdownDelay : 0;
Review Comment:
Perhaps validate that `waitForShutdownDelay` is a positive integer, if
specified, and throw an invalid input exception for the operator persona if not.
##########
extensions-contrib/prometheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/PrometheusEmitterTest.java:
##########
@@ -333,7 +341,61 @@ public void testEmitterConfig()
true,
true,
60,
+ null,
+ false,
null
);
}
+
+ @Test
+ public void testEmitterWithDeleteOnShutdown() throws IOException
+ {
+ PrometheusEmitterConfig emitterConfig = new
PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.pushgateway,
"namespace3", null, 0, "pushgateway", true, true, 60, null, true, null);
+
+ PushGateway mockPushGateway = mock(PushGateway.class);
+ mockPushGateway.push(anyObject(CollectorRegistry.class), anyString(),
anyObject(ImmutableMap.class));
+ expectLastCall().atLeastOnce();
+ mockPushGateway.delete(anyString(), anyObject(ImmutableMap.class));
+ expectLastCall().atLeastOnce();
Review Comment:
```suggestion
expectLastCall().once();
```
--
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]
