abhishekrb19 commented on code in PR #18598:
URL: https://github.com/apache/druid/pull/18598#discussion_r2422224037
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/Metrics.java:
##########
@@ -63,7 +63,7 @@ public DimensionsAndCollector getByName(String name, String
service)
}
}
- public Metrics(String namespace, String path, boolean isAddHostAsLabel,
boolean isAddServiceAsLabel, Map<String, String> extraLabels)
+ public Metrics(String namespace, String path, boolean isAddHostAsLabel,
boolean isAddServiceAsLabel, Map<String, String> extraLabels, Integer
ttlSeconds)
Review Comment:
```suggestion
public Metrics(String namespace, String path, boolean isAddHostAsLabel,
boolean isAddServiceAsLabel, Map<String, String> extraLabels, @Nullable Integer
ttlSeconds)
```
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/DimensionsAndCollector.java:
##########
@@ -20,20 +20,26 @@
package org.apache.druid.emitter.prometheus;
import io.prometheus.client.SimpleCollector;
+import org.apache.druid.java.util.common.Stopwatch;
+import org.joda.time.Duration;
public class DimensionsAndCollector
{
private final String[] dimensions;
private final SimpleCollector collector;
private final double conversionFactor;
private final double[] histogramBuckets;
+ private final Stopwatch updateTimer;
+ private final Integer ttlSeconds;
- DimensionsAndCollector(String[] dimensions, SimpleCollector collector,
double conversionFactor, double[] histogramBuckets)
+ DimensionsAndCollector(String[] dimensions, SimpleCollector collector,
double conversionFactor, double[] histogramBuckets, Integer ttlSeconds)
{
this.dimensions = dimensions;
this.collector = collector;
this.conversionFactor = conversionFactor;
this.histogramBuckets = histogramBuckets;
+ this.updateTimer = Stopwatch.createStarted();
+ this.ttlSeconds = ttlSeconds;
Review Comment:
nit: you could assign `Duration.standardSeconds(ttlSeconds)` in the
constructor itself
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/DimensionsAndCollector.java:
##########
@@ -20,20 +20,26 @@
package org.apache.druid.emitter.prometheus;
import io.prometheus.client.SimpleCollector;
+import org.apache.druid.java.util.common.Stopwatch;
+import org.joda.time.Duration;
public class DimensionsAndCollector
{
private final String[] dimensions;
private final SimpleCollector collector;
private final double conversionFactor;
private final double[] histogramBuckets;
+ private final Stopwatch updateTimer;
+ private final Integer ttlSeconds;
- DimensionsAndCollector(String[] dimensions, SimpleCollector collector,
double conversionFactor, double[] histogramBuckets)
+ DimensionsAndCollector(String[] dimensions, SimpleCollector collector,
double conversionFactor, double[] histogramBuckets, Integer ttlSeconds)
Review Comment:
```suggestion
DimensionsAndCollector(String[] dimensions, SimpleCollector collector,
double conversionFactor, double[] histogramBuckets, @Nullable Integer
ttlSeconds)
```
##########
website/.spelling:
##########
@@ -2456,3 +2456,6 @@ PropertyKeyName
PropertyValueType
tableSpec
TableSpec
+
+- ../docs/development/extensions-contrib/prometheus.md
Review Comment:
I think this isn't needed?
```suggestion
```
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -44,7 +44,7 @@ All the configuration parameters for the Prometheus emitter
are under `druid.emi
| `druid.emitter.prometheus.addHostAsLabel` | Flag to include the hostname
as a prometheus label.
| no | false
|
| `druid.emitter.prometheus.addServiceAsLabel` | Flag to include the druid
service name (e.g. `druid/broker`, `druid/coordinator`, etc.) as a prometheus
label.
| no | false
|
| `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.flushPeriod` | When using the `pushgateway`
strategy metrics are emitted every `flushPeriod` seconds. <br/>When using the
`exporter` strategy this configures the metric TTL such that if the metric
value is not updated within `flushPeriod` seconds then it will stop being
emitted. It is recommended to set this to at least 3 * `scrape_interval`. |
Required if `pushgateway` strategy is used, optional otherwise. | 15 seconds
for `pushgateway` strategy. <br/>None for `exporter` strategy. |
Review Comment:
An additional note on the behavior for clarity:
```suggestion
| `druid.emitter.prometheus.flushPeriod` | When using the
`pushgateway` strategy metrics are emitted every `flushPeriod` seconds.
<br/>When using the `exporter` strategy this configures the metric TTL such
that if the metric value is not updated within `flushPeriod` seconds then it
will stop being emitted. Note that unique label combinations per metric are
currently not subject to TTL expiration. It is recommended to set this to at
least 3 * `scrape_interval`. | Required if `pushgateway` strategy is used,
optional otherwise. | 15 seconds for `pushgateway` strategy. <br/>None for
`exporter` strategy. |
```
--
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]
