Optional removal of metrics from Prometheus PushGateway on shutdown #14935
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kfaraz
reviewed
Sep 1, 2023
...theus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java
Outdated
Show resolved
Hide resolved
abhishekrb19
reviewed
Sep 1, 2023
.../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.
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.
Yes, I add this in the following commit
…prometheus-pushgateway-metrics
abhishekrb19
reviewed
Dec 1, 2023
There was a problem hiding this comment.
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 |
abhishekrb19
approved these changes
Dec 10, 2023
.../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! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
pushgatewaystrategy 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.deletePushGatewayMetricsOnShutdownis 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
PrometheusEmitterPrometheusEmitterConfigThis PR has: