-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Optional removal of metrics from Prometheus PushGateway on shutdown #14935
Optional removal of metrics from Prometheus PushGateway on shutdown #14935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, @BartMiki !
I have left some minor comments, otherwise the changes look good to me.
...theus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java
Outdated
Show resolved
Hide resolved
.../prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java
Outdated
Show resolved
Hide resolved
| property | description | required? | default | | ||
|--------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------|--------------------------------------| | ||
| `druid.emitter.prometheus.strategy` | The strategy to expose prometheus metrics. <br/>Should be one of `exporter` and `pushgateway`. Default strategy `exporter` would expose metrics for scraping purpose. Peon tasks (short-lived jobs) should use `pushgateway` strategy. | yes | exporter | | ||
| `druid.emitter.prometheus.port` | The port on which to expose the prometheus HTTPServer. Required if using `exporter` strategy. | no | none | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please revert this table formatting change? It makes it hard to see what changed in the diff. If you sync master, it has Intellij settings for FORMAT_TABLES
that will not format and wrap long lines in the table automatically - https://github.com/apache/druid/blob/master/dev/druid_intellij_formatting.xml#L78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I add this in the following commit
…prometheus-pushgateway-metrics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I left a few comments and test cases to add. Thanks, @BartMiki!
.../prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java
Outdated
Show resolved
Hide resolved
.../prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java
Outdated
Show resolved
Hide resolved
...metheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/PrometheusEmitterTest.java
Show resolved
Hide resolved
...metheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/PrometheusEmitterTest.java
Show resolved
Hide resolved
...theus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java
Outdated
Show resolved
Hide resolved
@abhishekrb19 Hi! I update the PR based on your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some minor comments. Thanks, @BartMiki!
.../prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java
Show resolved
Hide resolved
...theus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java
Outdated
Show resolved
Hide resolved
@abhishekrb19 Thank you for the review! I've addressed your comments. Let me know if there is something else to change. |
@BartMiki, looks good -- thanks for the patch and addressing review feedback! |
Description
This PR adds an optional removal of Prometheus PushGateway metrics after the Prometheus emitter shutdown. The rationale for this change is that the PushGateway will remember all of the pushed metrics even if the batch job was finished a long time ago. This caused an issue with one of our clients as the PushGateway stored too many stale time series, that were already ingested by Prometheus.
We fix this issue within this repository https://github.com/deep-bi/druid-prometheus-emitter and we want to merge our fix with the Druid source code.
This PR only modifies the code of the Prometheus emitter extension. We extend a configuration with one more property and we add this information to the extension README.
Release note
Druid operators can specify optional prometheus emitter configs to delete metrics from the push gateway on task shutdown when the
pushgateway
strategy is configured:druid.emitter.prometheus.deletePushGatewayMetricsOnShutdown
: When set to true, the peon tasks deletes metrics from the prometheus push gateway on task shutdown. Default value is false.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 whendruid.emitter.prometheus.deletePushGatewayMetricsOnShutdown
is set to true. Default value is none - i.e., there is no delay between peon task shutdown and metrics deletion from the push gateway.Key changed/added classes in this PR
PrometheusEmitter
PrometheusEmitterConfig
This PR has: