You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "navina (via GitHub)" <gi...@apache.org> on 2023/05/08 18:50:59 UTC
[GitHub] [pinot] navina opened a new pull request, #10744: Add description field to metrics in Pinot
navina opened a new pull request, #10744:
URL: https://github.com/apache/pinot/pull/10744
This PR introduces a new `description` field in Pinot's metric abstraction model.
* This allows a developer to add metrics related description/documentation directly in the code
* We can further automate metrics documentation generation using a standalone script
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10744: Add description field to metrics in Pinot
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10744:
URL: https://github.com/apache/pinot/pull/10744#discussion_r1187798247
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java:
##########
@@ -101,11 +101,16 @@ public enum ServerMeter implements AbstractMetrics.Meter {
private final String _meterName;
private final String _unit;
private final boolean _global;
+ private final String _description;
ServerMeter(String unit, boolean global) {
+ this(unit, global, "");
+ }
Review Comment:
Nit: new line
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerGauge.java:
##########
@@ -54,10 +54,16 @@ public enum ServerGauge implements AbstractMetrics.Gauge {
private final String _unit;
private final boolean _global;
+ private final String _description;
+
ServerGauge(String unit, boolean global) {
+ this(unit, global, "");
+ }
Review Comment:
nit: new line
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMetrics.java:
##########
@@ -79,4 +79,10 @@ protected ServerMeter[] getMeters() {
protected ServerGauge[] getGauges() {
return ServerGauge.values();
}
+
+ public static void main(String[] args) {
Review Comment:
Remove this
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -67,4 +76,16 @@ public String getTimerName() {
public boolean isGlobal() {
return _global;
}
+
+ @Override
+ public String getDescription() {
+ return _description;
+ }
+
+ public static void main(String[] args) {
Review Comment:
Remove 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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #10744: Add description field to metrics in Pinot
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10744:
URL: https://github.com/apache/pinot/pull/10744
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on pull request #10744: Add description field to metrics in Pinot
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on PR #10744:
URL: https://github.com/apache/pinot/pull/10744#issuecomment-1538872590
@xiangfu0 / @Jackie-Jiang : Review pls!
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10744: Add description field to metrics in Pinot
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10744:
URL: https://github.com/apache/pinot/pull/10744#issuecomment-1539025116
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10744?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10744](https://app.codecov.io/gh/apache/pinot/pull/10744?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8940cf2) into [master](https://app.codecov.io/gh/apache/pinot/commit/815cd6ebb7f2236b127a4b7a7f8d59bea2c9a1cc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (815cd6e) will **decrease** coverage by `50.55%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10744 +/- ##
=============================================
- Coverage 64.38% 13.83% -50.55%
+ Complexity 6441 439 -6002
=============================================
Files 2068 2071 +3
Lines 111794 111907 +113
Branches 16956 16963 +7
=============================================
- Hits 71974 15481 -56493
- Misses 34646 95157 +60511
+ Partials 5174 1269 -3905
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `13.83% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10744?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/pinot/common/metrics/AbstractMetrics.java](https://app.codecov.io/gh/apache/pinot/pull/10744?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9BYnN0cmFjdE1ldHJpY3MuamF2YQ==) | `0.00% <0.00%> (-40.71%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/ServerGauge.java](https://app.codecov.io/gh/apache/pinot/pull/10744?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJHYXVnZS5qYXZh) | `0.00% <0.00%> (-93.11%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://app.codecov.io/gh/apache/pinot/pull/10744?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `0.00% <0.00%> (-98.67%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/ServerTimer.java](https://app.codecov.io/gh/apache/pinot/pull/10744?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJUaW1lci5qYXZh) | `0.00% <0.00%> (-84.62%)` | :arrow_down: |
... and [1442 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10744/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10744: Add description field to metrics in Pinot
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10744:
URL: https://github.com/apache/pinot/pull/10744#discussion_r1188964395
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -93,6 +93,10 @@ public PinotMetricsRegistry getMetricsRegistry() {
public interface QueryPhase {
String getQueryPhaseName();
+
+ default String getDescription() {
+ return "";
+ }
Review Comment:
why do we need the default behavior if the constructor is already catching and setting `""` ? ( are we concern backward-compatibility for other 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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #10744: Add description field to metrics in Pinot
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10744:
URL: https://github.com/apache/pinot/pull/10744#discussion_r1189048094
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -93,6 +93,10 @@ public PinotMetricsRegistry getMetricsRegistry() {
public interface QueryPhase {
String getQueryPhaseName();
+
+ default String getDescription() {
+ return "";
+ }
Review Comment:
technically, it is a public interface. So, we never know who and all have extended this interface to implement their own metrics. So, it is left there for compatibility.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on pull request #10744: Add description field to metrics in Pinot
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on PR #10744:
URL: https://github.com/apache/pinot/pull/10744#issuecomment-1540771392
> Is auto documentation generation the primary motivation? (Not for this PR, general question) Is there a way to extract that field in our prometheus config files, so it can be used in our observability tools?
documentation and auto-generation of monitoring files (like the ones we use with prometheus export). But that requires a little bit more work to get there. In the mean time, I wanted to kickstart the practice of making documentation as a part of the code.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org