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/05 12:50:43 UTC

[GitHub] [druid] FrankChen021 opened a new pull request, #13028: Improve doc and configuration of prometheus emitter

FrankChen021 opened a new pull request, #13028:
URL: https://github.com/apache/druid/pull/13028

   I noticed that there are many issues raised about configuration of prometheus emitter. Current doc is not clear on how to correctly configure it to emit metrics of peon tasks.
   
   Also configuration constratins are consolidated to satisified with the doc.
   
   
   This PR has:
   - [X] been self-reviewed.
      - [ ] 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.
   - [ ] 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.
   - [ ] 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 #13028: Improve doc and configuration of prometheus emitter

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13028:
URL: https://github.com/apache/druid/pull/13028#discussion_r965437390


##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -28,22 +28,38 @@ To use this Apache Druid extension, [include](../../development/extensions.md#lo
 ## Introduction
 
 This extension exposes [Druid metrics](https://druid.apache.org/docs/latest/operations/metrics.html) for collection by a Prometheus server (https://prometheus.io/).
+
 Emitter is enabled by setting `druid.emitter=prometheus` [configs](https://druid.apache.org/docs/latest/configuration/index.html#emitting-metrics) or include `prometheus` in the composing emitter list. 
 
 ## Configuration
 
 All the configuration parameters for the Prometheus emitter are under `druid.emitter.prometheus`.
 
-|property|description|required?|default|
-|--------|-----------|---------|-------|
-|`druid.emitter.prometheus.strategy`|The strategy to expose prometheus metrics. Default strategy `exporter` would expose metrics for scraping purpose. Only peon task (short-lived jobs) need to 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|
-|`druid.emitter.prometheus.namespace`|Optional metric namespace. Must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*`|no|"druid"|
-|`druid.emitter.prometheus.dimensionMapPath`|JSON file defining the Prometheus metric type, desired dimensions, help text, and conversionFactor for every Druid metric.|no|Default mapping provided. See below.|
-|`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|
+| property                                      | description                                                                                                                                                                                                                            | required? | default                              |

Review Comment:
   Intellij formats this for meπŸ˜„
   I know this bring a little difficulty for review and I will try to keep the orignal format in future change.



-- 
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 #13028: Improve doc and configuration of prometheus emitter

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13028:
URL: https://github.com/apache/druid/pull/13028#issuecomment-1239418726

   Thank you @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] kfaraz commented on pull request #13028: Improve doc and configuration of prometheus emitter

Posted by GitBox <gi...@apache.org>.
kfaraz commented on PR #13028:
URL: https://github.com/apache/druid/pull/13028#issuecomment-1239411911

   Sure, @FrankChen021 , I will take a look today.


-- 
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 #13028: Improve doc and configuration of prometheus emitter

Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13028:
URL: https://github.com/apache/druid/pull/13028#discussion_r964920670


##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -58,15 +74,34 @@ be provided as a JSON file.  Additionally, this mapping specifies which dimensio
 histogram timers to use Seconds as the base unit.  Timers which do not use seconds as a base unit can use the `conversionFactor` to set
 the base time unit. If the user does not specify their own JSON file, a default mapping is used.  All
 metrics are expected to be mapped. Metrics which are not mapped will not be tracked.
+
 Prometheus metric path is organized using the following schema:
-`<druid metric name> : { "dimensions" : <dimension list>, "type" : <timer|counter|gauge>, conversionFactor: <conversionFactor>, "help" : <help text>,}`
-e.g.
-`query/time" : { "dimensions" : ["dataSource", "type"], "conversionFactor": 1000.0, "type" : "timer", "help": "Seconds taken to complete a query."}`
+
+```json
+<druid metric name> : { 
+  "dimensions" : <dimension list>, 
+  "type" : <timer|counter|gauge>, 
+  "conversionFactor": <conversionFactor>, 
+  "help" : <help text>
+}
+```
+
+For example:
+```json
+"query/time" : { 
+  "dimensions" : ["dataSource", "type"],
+  "type" : "timer",
+  "conversionFactor": 1000.0,
+  "help": "Seconds taken to complete a query."
+}
+```
 
 For metrics which are emitted from multiple services with different dimensions, the metric name is prefixed with
-the service name.
-e.g.
-`"coordinator-segment/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
- "historical-segment/count" : { "dimensions" : ["dataSource", "tier", "priority"], "type" : "gauge" }`
+the service name. For example:
+
+```json
+"coordinator-segment/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
+"historical-segment/count" : { "dimensions" : ["dataSource", "tier", "priority"], "type" : "gauge" }
+```
  
-For most use-cases, the default mapping is sufficient.
+For most use cases, the default mapping is sufficient.

Review Comment:
   Good one πŸ˜„ !



##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -28,22 +28,38 @@ To use this Apache Druid extension, [include](../../development/extensions.md#lo
 ## Introduction
 
 This extension exposes [Druid metrics](https://druid.apache.org/docs/latest/operations/metrics.html) for collection by a Prometheus server (https://prometheus.io/).
+
 Emitter is enabled by setting `druid.emitter=prometheus` [configs](https://druid.apache.org/docs/latest/configuration/index.html#emitting-metrics) or include `prometheus` in the composing emitter list. 
 
 ## Configuration
 
 All the configuration parameters for the Prometheus emitter are under `druid.emitter.prometheus`.
 
-|property|description|required?|default|
-|--------|-----------|---------|-------|
-|`druid.emitter.prometheus.strategy`|The strategy to expose prometheus metrics. Default strategy `exporter` would expose metrics for scraping purpose. Only peon task (short-lived jobs) need to 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|
-|`druid.emitter.prometheus.namespace`|Optional metric namespace. Must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*`|no|"druid"|
-|`druid.emitter.prometheus.dimensionMapPath`|JSON file defining the Prometheus metric type, desired dimensions, help text, and conversionFactor for every Druid metric.|no|Default mapping provided. See below.|
-|`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|
+| 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                                 |
+| `druid.emitter.prometheus.namespace`          | Optional metric namespace. Must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*`                                                                                                                                                             | no        | druid                                |
+| `druid.emitter.prometheus.dimensionMapPath`   | JSON file defining the Prometheus metric type, desired dimensions, help text, and conversionFactor for every Druid metric.                                                                                                             | no        | Default mapping provided. See below. |
+| `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                                 |
+
+### Override properties for Peon Tasks
+
+Since peon tasks are created dynamically by MiddleManagers, it's not able to use `exporter` strategy for these tasks to let prometheus read metrics from fixed addresses.

Review Comment:
   ```suggestion
   Peon tasks are created dynamically by middle managers and have dynamic host and port addresses. Since the `exporter` strategy allows Prometheus to read only from a fixed address, it cannot be used for peon tasks.
   ```
   
   Tried to simplify the language a bit.



##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -28,22 +28,38 @@ To use this Apache Druid extension, [include](../../development/extensions.md#lo
 ## Introduction
 
 This extension exposes [Druid metrics](https://druid.apache.org/docs/latest/operations/metrics.html) for collection by a Prometheus server (https://prometheus.io/).
+
 Emitter is enabled by setting `druid.emitter=prometheus` [configs](https://druid.apache.org/docs/latest/configuration/index.html#emitting-metrics) or include `prometheus` in the composing emitter list. 
 
 ## Configuration
 
 All the configuration parameters for the Prometheus emitter are under `druid.emitter.prometheus`.
 
-|property|description|required?|default|
-|--------|-----------|---------|-------|
-|`druid.emitter.prometheus.strategy`|The strategy to expose prometheus metrics. Default strategy `exporter` would expose metrics for scraping purpose. Only peon task (short-lived jobs) need to 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|
-|`druid.emitter.prometheus.namespace`|Optional metric namespace. Must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*`|no|"druid"|
-|`druid.emitter.prometheus.dimensionMapPath`|JSON file defining the Prometheus metric type, desired dimensions, help text, and conversionFactor for every Druid metric.|no|Default mapping provided. See below.|
-|`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|
+| 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                                 |
+| `druid.emitter.prometheus.namespace`          | Optional metric namespace. Must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*`                                                                                                                                                             | no        | druid                                |
+| `druid.emitter.prometheus.dimensionMapPath`   | JSON file defining the Prometheus metric type, desired dimensions, help text, and conversionFactor for every Druid metric.                                                                                                             | no        | Default mapping provided. See below. |
+| `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                                 |
+
+### Override properties for Peon Tasks
+
+Since peon tasks are created dynamically by MiddleManagers, it's not able to use `exporter` strategy for these tasks to let prometheus read metrics from fixed addresses.
+So, these tasks need to be configured to use `pushgateway` strategy to push metrics from Druid to prometheus gateway.
+
+If this emitter is configured to use `exporter` strategy globally, some above configurations need to be overridden in the MiddleManager's configuration file for peon tasks as shown below.

Review Comment:
   ```suggestion
   If this emitter is configured to use `exporter` strategy globally, some of the above configurations need to be overridden in the middle manager so that spawned peon tasks can still use the `pushgateway` strategy.
   ```



##########
docs/development/extensions-contrib/prometheus.md:
##########
@@ -28,22 +28,38 @@ To use this Apache Druid extension, [include](../../development/extensions.md#lo
 ## Introduction
 
 This extension exposes [Druid metrics](https://druid.apache.org/docs/latest/operations/metrics.html) for collection by a Prometheus server (https://prometheus.io/).
+
 Emitter is enabled by setting `druid.emitter=prometheus` [configs](https://druid.apache.org/docs/latest/configuration/index.html#emitting-metrics) or include `prometheus` in the composing emitter list. 
 
 ## Configuration
 
 All the configuration parameters for the Prometheus emitter are under `druid.emitter.prometheus`.
 
-|property|description|required?|default|
-|--------|-----------|---------|-------|
-|`druid.emitter.prometheus.strategy`|The strategy to expose prometheus metrics. Default strategy `exporter` would expose metrics for scraping purpose. Only peon task (short-lived jobs) need to 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|
-|`druid.emitter.prometheus.namespace`|Optional metric namespace. Must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*`|no|"druid"|
-|`druid.emitter.prometheus.dimensionMapPath`|JSON file defining the Prometheus metric type, desired dimensions, help text, and conversionFactor for every Druid metric.|no|Default mapping provided. See below.|
-|`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|
+| property                                      | description                                                                                                                                                                                                                            | required? | default                              |

Review Comment:
   Style:
   I think I prefer the original formatting style. Having the columns aligned is good, but it becomes difficult to read once the text begins to wrap. Content written in the older style can be easily read through without having to consciously skip the whitespaces.



-- 
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 #13028: Improve doc and configuration of prometheus emitter

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13028:
URL: https://github.com/apache/druid/pull/13028#issuecomment-1239409494

   @kfaraz Could you review this?


-- 
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 #13028: Improve doc and configuration of prometheus emitter

Posted by GitBox <gi...@apache.org>.
FrankChen021 merged PR #13028:
URL: https://github.com/apache/druid/pull/13028


-- 
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