You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2024/11/26 13:23:05 UTC
[PR] Normalized metric names, added descriptions to documentation [accumulo]
dlmarion opened a new pull request, #5112:
URL: https://github.com/apache/accumulo/pull/5112
Renamed MetricCategory to MetricDocSection. Updated MetricDocSection enum values to include a description which is added to each metric section in the generated documentation. Renamed metrics, where applicable, to remove the location from which they are emitted as that is included in the tags emitted with the metric. Tried to use a naming convention for the metrics that describe the function being measured, not where it's measured.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalized metric names, added descriptions to documentation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5112:
URL: https://github.com/apache/accumulo/pull/5112#discussion_r1859099152
##########
core/src/main/java/org/apache/accumulo/core/metrics/Metric.java:
##########
@@ -304,36 +308,49 @@ public String getDescription() {
return description;
}
- public MetricCategory getCategory() {
- return category;
+ public MetricDocSection getCategory() {
Review Comment:
I thought that MetricCategory was too vague and didn't convey the manner in which the value was used. MetricCategory / MetricDocSection is only used in MetricsDocGen.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalized metric names, added descriptions to documentation [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5112:
URL: https://github.com/apache/accumulo/pull/5112#discussion_r1859126639
##########
core/src/main/java/org/apache/accumulo/core/metrics/Metric.java:
##########
@@ -304,36 +308,49 @@ public String getDescription() {
return description;
}
- public MetricCategory getCategory() {
- return category;
+ public MetricDocSection getDocSection() {
+ return section;
}
public enum MetricType {
GAUGE, COUNTER, TIMER, LONG_TASK_TIMER, DISTRIBUTION_SUMMARY, FUNCTION_COUNTER, CACHE
}
- public enum MetricCategory {
- GENERAL_SERVER("General Server Metrics"),
- COMPACTION("Compaction Metrics"),
- COMPACTOR("Compactor Metrics"),
- FATE("Fate Metrics"),
- GARBAGE_COLLECTION("Garbage Collection Metrics"),
- TABLET_SERVER("Tablet Server Metrics"),
- SCAN("Scan Metrics"),
- SCAN_SERVER("Scan Server Metrics"),
- THRIFT("Thrift Metrics"),
- BLOCK_CACHE("Block Cache Metrics"),
- MANAGER("Manager Metrics");
+ public enum MetricDocSection {
+ GENERAL_SERVER("General Server Metrics", "Metrics that are generated across all server types."),
+ COMPACTION("Compaction Metrics",
+ "Metrics specific to compactions, both minor and major. Metrics for major compactions"
+ + " will likely have a 'queue.id' tag. The CompactionCoordinator component in the Manager creates a queue for each CompactionService"
+ + " in the configuration. The 'queue.id' tag may map directly to the name of a Compactor resource group."),
+ COMPACTOR("Compactor Metrics", "Metrics that are generated by the Compactor processes."),
+ FATE("Fate Metrics",
+ "Metrics that are generated by the Fate component in the Manager process."),
+ GARBAGE_COLLECTION("Garbage Collection Metrics",
+ "Metrics that are generated by the Garbage Collector process"),
Review Comment:
```suggestion
"Metrics that are generated by the Garbage Collector process."),
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalized metric names, added descriptions to documentation [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5112:
URL: https://github.com/apache/accumulo/pull/5112#discussion_r1859046386
##########
core/src/main/java/org/apache/accumulo/core/metrics/Metric.java:
##########
@@ -304,36 +308,49 @@ public String getDescription() {
return description;
}
- public MetricCategory getCategory() {
- return category;
+ public MetricDocSection getCategory() {
Review Comment:
What is the motivation behind changing this name? It seems like renaming it to reference the docs ties it too closely to the document generation context, potentially limiting its use in other parts of the code. This makes it seem specific to documentation purposes, even though the Metric enum is used elsewhere. It appears to mix responsibilities with the DocGen class.
##########
core/src/main/java/org/apache/accumulo/core/metrics/Metric.java:
##########
@@ -304,36 +308,49 @@ public String getDescription() {
return description;
}
- public MetricCategory getCategory() {
- return category;
+ public MetricDocSection getCategory() {
+ return section;
}
public enum MetricType {
GAUGE, COUNTER, TIMER, LONG_TASK_TIMER, DISTRIBUTION_SUMMARY, FUNCTION_COUNTER, CACHE
}
- public enum MetricCategory {
- GENERAL_SERVER("General Server Metrics"),
- COMPACTION("Compaction Metrics"),
- COMPACTOR("Compactor Metrics"),
- FATE("Fate Metrics"),
- GARBAGE_COLLECTION("Garbage Collection Metrics"),
- TABLET_SERVER("Tablet Server Metrics"),
- SCAN("Scan Metrics"),
- SCAN_SERVER("Scan Server Metrics"),
- THRIFT("Thrift Metrics"),
- BLOCK_CACHE("Block Cache Metrics"),
- MANAGER("Manager Metrics");
+ public enum MetricDocSection {
+ GENERAL_SERVER("General Server Metrics", "Metrics that are generated across all server types"),
Review Comment:
```suggestion
GENERAL_SERVER("General Server Metrics", "Metrics that are generated across all server types."),
```
Should make sure that all of the sentences end with a period.
##########
core/src/main/java/org/apache/accumulo/core/metrics/MetricsDocGen.java:
##########
@@ -52,21 +52,27 @@ void pageHeader() {
doc.println("---\n");
doc.println("<!-- WARNING: Do not edit this file. It is a generated file"
+ " that is copied from Accumulo build (from core/target/generated-docs) -->\n");
- doc.println("Below are the metrics used to monitor various components of Accumulo.\n");
+ doc.println(
+ "Below are the metrics used to monitor various components of Accumulo. Metrics emitted by Accumulo should"
+ + " contain the folowing tags: 'instance.name', 'resource.group', 'process.name', 'host' and 'port'. Metrics"
+ + " emitted by Accumulo may contain additional tags where we think it makes sense to capture per-object metrics,"
+ + " for example on a table or tablet basis in the ScanServer and TabletServer, or on a per-queue basis in the"
+ + " CompactionCoordinator.\n");
}
void generateTableOfContents() {
doc.println("## Table of Contents\n");
- for (var category : Metric.MetricCategory.values()) {
+ for (var category : Metric.MetricDocSection.values()) {
String sectionId = generateSectionId(category.getSectionTitle());
doc.println("- [" + category.getSectionTitle() + "](#" + sectionId + ")");
}
doc.println();
}
- void generateCategorySection(Metric.MetricCategory category, String sectionTitle) {
+ void generateCategorySection(Metric.MetricDocSection category, String sectionTitle,
Review Comment:
It could make sense to just accept the `Metric.MetricDocSection category` as the only argument here and use it to get the title and description. That would make for a bit cleaner code probably. Also, could update the name of `category` in the method and var names since its changed to section.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalized metric names, added descriptions to documentation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5112:
URL: https://github.com/apache/accumulo/pull/5112#discussion_r1859116089
##########
core/src/main/java/org/apache/accumulo/core/metrics/MetricsDocGen.java:
##########
@@ -52,21 +52,27 @@ void pageHeader() {
doc.println("---\n");
doc.println("<!-- WARNING: Do not edit this file. It is a generated file"
+ " that is copied from Accumulo build (from core/target/generated-docs) -->\n");
- doc.println("Below are the metrics used to monitor various components of Accumulo.\n");
+ doc.println(
+ "Below are the metrics used to monitor various components of Accumulo. Metrics emitted by Accumulo should"
+ + " contain the folowing tags: 'instance.name', 'resource.group', 'process.name', 'host' and 'port'. Metrics"
+ + " emitted by Accumulo may contain additional tags where we think it makes sense to capture per-object metrics,"
+ + " for example on a table or tablet basis in the ScanServer and TabletServer, or on a per-queue basis in the"
+ + " CompactionCoordinator.\n");
}
void generateTableOfContents() {
doc.println("## Table of Contents\n");
- for (var category : Metric.MetricCategory.values()) {
+ for (var category : Metric.MetricDocSection.values()) {
String sectionId = generateSectionId(category.getSectionTitle());
doc.println("- [" + category.getSectionTitle() + "](#" + sectionId + ")");
}
doc.println();
}
- void generateCategorySection(Metric.MetricCategory category, String sectionTitle) {
+ void generateCategorySection(Metric.MetricDocSection category, String sectionTitle,
Review Comment:
Took care of this in 27cd47b
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalized metric names, added descriptions to documentation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #5112:
URL: https://github.com/apache/accumulo/pull/5112
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org