You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/09/06 16:49:02 UTC
[GitHub] [druid] 599166320 opened a new pull request, #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
599166320 opened a new pull request, #13034:
URL: https://github.com/apache/druid/pull/13034
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
Fixes [#13025](https://github.com/apache/druid/issues/13025).
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### Add a scheduler to support sending metrics to the pushgateway regularly and continuously
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
##### Key changed/added classes in this PR
* `PrometheusEmitter`
* `PrometheusEmitterConfig`
* `PrometheusEmitterTest`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [x] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r965572835
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -43,7 +43,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|
Review Comment:
`flushPeriod` is what I suggest because it's the same property name used in graphite emitter.
I think maybe we can unify some configurations and the scheduler among all emitters in someday. @kfaraz
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r965434233
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -43,7 +43,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 using Pushgateway strategy|no|15|
+|`druid.emitter.prometheus.flushPeriod`|Emit metrics to Pushgateway every `flushPeriod` seconds. Required if `Pushgateway` strategy is used.|no|15|
Review Comment:
```suggestion
|`druid.emitter.prometheus.flushPeriod`|Emit metrics to Pushgateway every `flushPeriod` seconds. Required if `pushgateway` strategy is used.|no|15|
```
I think it should be in lower case to match the name used in line 39
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240756583
> Sorry, I see now that I did not use precise language. My concern was not the map of registered metrics itself, because once initialized, it is never updated, only read from. But it does not hurt to have this as an unmodifiable map.
>
> My actual concern was regarding `DimensionsAndCollector` objects.
>
> 1. Let's say there are 2 registered metrics.
> 2. A `flush` event comes and the thread starts to read the metrics and has finished reading the value of the first metric.
> 3. Then `emit` event comes and updates the value of the second metric in the respective `DimensionsAndCollector` object.
> 4. `flush` thread then reads the value of the second metric.
> 5. `flush` thread pushes the old value of first metric but a new value of second metric. Thus the metric packet is inconsistent. I would rather the packet had either all old values or all new values.
In promethues, if multiple metrics are related, histogram and summary are generally used. These composite types contain multiple thinnest metrics inside.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240544273
> The changes look good. I have some concerns about thread-safety though. The `Metrics` object or the map of `registeredMetrics` inside it is not thread-safe. Every invocation of `emit` updates the map of registered which is being read by `flush` on a different thread. So it's possible that the metric map gets updated by an `emit` event while a `flush` event is still reading from the map, thus sending a metric packet with inconsistent values.
>
> I am not sure it would be a good idea to block `emit` while `flush` is going on either.
>
> @599166320 , @FrankChen021 , what do you think?
>
I think we should initialize registeredMetrics in the constructor to ensure that it cannot be modified anywhere outside the constructor,
like the following code:
```
public Metrics(String namespace, String path, boolean isAddHostAsLabel, boolean isAddServiceAsLabel)
{
Map<String, DimensionsAndCollector> registeredMetricsTmp = new HashMap<>();
Map<String, Metric> metrics = readConfig(path);
for (Map.Entry<String, Metric> entry : metrics.entrySet()) {
//......
if (collector != null) {
registeredMetricsTmp.put(name, new DimensionsAndCollector(dimensions, collector, metric.conversionFactor));
}
}
this.registeredMetrics = Collections.unmodifiableMap(registeredMetricsTmp);
}
```
What do you think?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240508471
>
I'll have a look later
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r965532322
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java:
##########
@@ -91,6 +95,13 @@ public void start()
} else {
pushGateway = new PushGateway(address);
}
+ exec = ScheduledExecutors.fixed(1, "PrometheusPushGatewayEmitter-%s");
Review Comment:
> The executor should also be shutdown, maybe in `close`?
Yes, I almost forgot
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r964584440
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -78,8 +83,10 @@ public PrometheusEmitterConfig(
this.port = port;
if (this.strategy == Strategy.pushgateway) {
Preconditions.checkNotNull(pushGatewayAddress, "Invalid pushGateway address");
+ Preconditions.checkNotNull(pushRateSec, "Invalid pushRateSec");
Review Comment:
I changed the default value of flushPeriod to 15s, because the default scrape_interval of prometheus is the same
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240548397
> > The changes look good. I have some concerns about thread-safety though. The `Metrics` object or the map of `registeredMetrics` inside it is not thread-safe. Every invocation of `emit` updates the map of registered which is being read by `flush` on a different thread. So it's possible that the metric map gets updated by an `emit` event while a `flush` event is still reading from the map, thus sending a metric packet with inconsistent values.
> > I am not sure it would be a good idea to block `emit` while `flush` is going on either.
> > @599166320 , @FrankChen021 , what do you think?
>
> >
>
> I think we should initialize registeredMetrics in the constructor to ensure that it cannot be modified anywhere outside the constructor, like the following code:
>
> ```
> public Metrics(String namespace, String path, boolean isAddHostAsLabel, boolean isAddServiceAsLabel)
> {
> Map<String, DimensionsAndCollector> registeredMetricsTmp = new HashMap<>();
> Map<String, Metric> metrics = readConfig(path);
> for (Map.Entry<String, Metric> entry : metrics.entrySet()) {
> //......
> if (collector != null) {
> registeredMetricsTmp.put(name, new DimensionsAndCollector(dimensions, collector, metric.conversionFactor));
> }
> }
> this.registeredMetrics = Collections.unmodifiableMap(registeredMetricsTmp);
> }
> ```
>
> What do you think?
Looks good to me.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1241073286
Hi @599166320 Since there's a CI failure in this PR, I merged #13028 in advance which brought some conflicts with what you have done in this PR.
Appreciate that if you can resolve these conflicts by merging the master to your branch. Sorry for the inconvenience caused.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r964399372
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -78,8 +83,10 @@ public PrometheusEmitterConfig(
this.port = port;
if (this.strategy == Strategy.pushgateway) {
Preconditions.checkNotNull(pushGatewayAddress, "Invalid pushGateway address");
+ Preconditions.checkNotNull(pushRateSec, "Invalid pushRateSec");
Review Comment:
Can this property have a default value if it's not configured by user?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r964398539
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -53,6 +53,10 @@
@Nullable
private final String pushGatewayAddress;
+ @JsonProperty
+ @Nullable
+ private final Integer pushRateSec;
Review Comment:
Better to name it as `flushPeriod` since it's the same property name in other emitter extensions.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r964852589
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -43,7 +43,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 using Pushgateway strategy|no|15|
Review Comment:
```suggestion
|`druid.emitter.prometheus.flushPeriod`|Emit metrics to Pushgateway every `flushPeriod` seconds. Required if `pushgateway` strategy is used. |no|15|
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r965515787
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java:
##########
@@ -91,6 +95,13 @@ public void start()
} else {
pushGateway = new PushGateway(address);
}
+ exec = ScheduledExecutors.fixed(1, "PrometheusPushGatewayEmitter-%s");
Review Comment:
The executor should also be shutdown, maybe in `close`?
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -43,7 +43,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|
Review Comment:
Also, the default value used by HttpEmitter is 60 seconds.
##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -43,7 +43,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|
Review Comment:
Maybe use `flushMillis` instead as http emitter seems to be using, so that there is no ambiguity of the unit.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240509396
@kfaraz Good point.
I looked through the code, the underlying `registeredMetrics` is intialized when `PrometheusEmitter` is initialized and the map won't change after that. When `emit` is called, it just gets value from the map, and does some change on the value object, but not the map object itself. So I think there's no concurrency problem.
But to prevent potential map modification, I'm suggesting to change the `getRegisteredMetrics` to return a unmodifiable map like
```java
public Map<String, DimensionsAndCollector> getRegisteredMetrics()
{
return Collections.unmodifiableMap(registeredMetrics);
}
```
Correct me if I'm wrong.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r964578703
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -53,6 +53,10 @@
@Nullable
private final String pushGatewayAddress;
+ @JsonProperty
+ @Nullable
+ private final Integer pushRateSec;
Review Comment:
ok, looks good
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1241816045
Failed task has been re-triggered. Let's wait for the result.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r966001181
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/Metrics.java:
##########
@@ -63,6 +64,7 @@ public DimensionsAndCollector getByName(String name, String service)
public Metrics(String namespace, String path, boolean isAddHostAsLabel, boolean isAddServiceAsLabel)
{
+ Map<String, DimensionsAndCollector> registeredMetricsTmp = new HashMap<>();
Review Comment:
It's OK. There are indeed many places in Java that like this usage style.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] 599166320 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
599166320 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1241662102
It seems that Travis CI needs to be triggered again.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
kfaraz commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240624863
Sorry, I see now that I did not use precise language. My concern was not the map of registered metrics itself, because once initialized, it is never updated, only read from. But it does not hurt to have this as an unmodifiable map.
My actual concern was regarding `DimensionsAndCollector` objects.
1. Let's say there are 2 registered metrics.
2. A `flush` event comes and the thread starts to read the metrics and has finished reading the value of the first metric.
3. Then `emit` event comes and updates the value of the second metric.
4. `flush` thread then reads the value of the second metric.
5. `flush` thread pushes the old value of first metric but a new value of second metric. Thus the metric packet is inconsistent. I would rather the packet had either all old values or all new values.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
kfaraz commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240744432
Thanks for the clarification, @FrankChen021 !
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240729090
Each metric is independent in prometheus, I think it's OK that first metric is with the old value and 2nd is with the new.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13034:
URL: https://github.com/apache/druid/pull/13034#discussion_r965997237
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/Metrics.java:
##########
@@ -63,6 +64,7 @@ public DimensionsAndCollector getByName(String name, String service)
public Metrics(String namespace, String path, boolean isAddHostAsLabel, boolean isAddServiceAsLabel)
{
+ Map<String, DimensionsAndCollector> registeredMetricsTmp = new HashMap<>();
Review Comment:
Nit: It is fine to name this as `registeredMetrics` as you would anyway be referring to the field by using `this.registeredMetrics`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
kfaraz commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240339747
The changes look good. I have some concerns about thread-safety though. The `Metrics` object or the map of `registeredMetrics` inside it is not thread-safe. Every invocation of `emit` updates the map of registered which is being read by `flush` on a different thread. So it's possible that the metric map gets updated by an `emit` event while a `flush` event is still reading from the map, thus sending a metric packet with inconsistent values.
I am not sure it would be a good idea to block `emit` while `flush` is going on either.
@599166320 , @FrankChen021 , what do you think?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 merged pull request #13034: prometheus-emitter supports sending metrics to pushgateway regularly …
Posted by GitBox <gi...@apache.org>.
FrankChen021 merged PR #13034:
URL: https://github.com/apache/druid/pull/13034
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org