Dynamically generate metrics documentation

[DomGarguilo]

Fixes #4815

• Creates a new class - Metrics, which is similar to Property where we can add documentation for each metric
• Creates a new MetricsDocGen class which pulls the data from Metrics and constructs a markdown document from it

[DomGarguilo]

Opening this PR as draft to hopefully get some feedback on this approach to documenting the metrics. This code is largely modeled after and works the same as the Property and ConfigurationDocGen classes. If this approach looks good I suspect we could probably refactor some shared logic into a parent class but wanted to make sure things look good before spending more time on it.

Here is an example of the output from the few example metrics I added to Metrics in 880411b:

Click to expand

Below are the metrics used to monitor various components of Accumulo.

General Server Metrics

accumulo.detected.low.memory

Type: GAUGE

Description: reports 1 when process memory usage is above threshold, 0 when memory is okay

accumulo.server.idle

Type: GAUGE

Description: Indicates if the server is idle or not. The value will be 1 when idle and 0 when not idle.

Compactor Metrics

Scan Server Metrics

accumulo.scan.busy.timeout.count

Type: COUNTER

Description: Count of the scans where a busy timeout happened

Fate Metrics

[DomGarguilo]

Marking this as ready for review since I think its mostly done.

One thing I thought of is if we want to use the description from the enum when registering the corresponding metric. For example:

accumulo/server/tserver/src/main/java/org/apache/accumulo/tserver/BlockCacheMetrics.java

Lines 57 to 58 in 3a265ed

	FunctionCounter.builder(BLOCKCACHE_INDEX_HITCOUNT.getName(), indexCache, getHitCount)
	.description("Index block cache hit count").register(registry);

could be changed to

 FunctionCounter.builder(BLOCKCACHE_INDEX_HITCOUNT.getName(), indexCache, getHitCount) 
     .description(BLOCKCACHE_INDEX_HITCOUNT.getDescription()).register(registry); 

this would make the description that appears in the docs the same as the one connected to the micrometer instrument. I am guessing we want to keep the description attached to the micrometer instrument more brief than the one that will appear on the docs but I figured I would ask.

[kevinrr888]

Some comments and questions. Overall, looks like a really good improvement

[DomGarguilo]

@kevinrr888 All of your suggestions should be addressed now.

[keith-turner]

FunctionCounter.builder(BLOCKCACHE_INDEX_HITCOUNT.getName(), indexCache, getHitCount) 
  .description(BLOCKCACHE_INDEX_HITCOUNT.getDescription()).register(registry); 

this would make the description that appears in the docs the same as the one connected to the micrometer instrument.

@DomGarguilo that would be really nice, is there a follow on issue for that?

[DomGarguilo]

FunctionCounter.builder(BLOCKCACHE_INDEX_HITCOUNT.getName(), indexCache, getHitCount) 
  .description(BLOCKCACHE_INDEX_HITCOUNT.getDescription()).register(registry); 

this would make the description that appears in the docs the same as the one connected to the micrometer instrument.

@DomGarguilo that would be really nice, is there a follow on issue for that?

Not yet. I can create one though. Two things I am thinking of that should be considered before consolidating these two descriptions though:

1. Would we want to keep the description attached to the Micrometer instrument shorter than we would want the description that will appear in the markdown doc? This may be an argument for always keeping the description very breif (yet descriptive) but also adding more fields that further describe or explain the metric (as @dlmarion and @ctubbsii were discussing above).
2. Similarly, if we want to take advantage of markdown styling within the descriptions, would that work okay for the descriptions attached to the Micrometer instrument?

[keith-turner]

Two things I am thinking of that should be considered before consolidating these two descriptions though:

For point number one we need to figure out what the implications are if any of a "long" description for micrometer. I am assuming long means more than a sentence or two. If there are no known negative implications, then should probably not worry about it.

For point number two, markdown is meant to be readable in source form. So as long as the characters used by markdown do not cause problems for micrometer then it seems ok to include it.

[DomGarguilo]

@keith-turner sounds good. I created #4870 to track this.

[DomGarguilo]

I made a PR to the website to add the doc generated by these changes: apache/accumulo-website#442.

Also, here is a screenshot of what things look like currently on the User manual page of the website when I run it from the linked branch:

[DomGarguilo]

I think this PR is ready to be merged as far as I can tell. Once this is merged I plan to open a follow on PR in the hopes that everyone can contribute to adding/editing the descriptions for each metric.

I have also verified that running mvn package -DskipTests correctly generates the markdown doc and puts in alongside the other generated markdown docs for client and server properties.

[dlmarion]

Was there already a discussion about the table format that is used for the properties? If so, I'll go read that.

[DomGarguilo]

Was there already a discussion about the table format that is used for the properties? If so, I'll go read that.

Here is a link to where I proposed the current format: #4815 (comment) along with the comment that follows it. That was pretty much the whole discussion on the formatting.

There is still time for more input/opinions as that is something that is pretty easy to change at this point. Do you a table better than how it is now with different sections per metric? @dlmarion

[dlmarion]

@DomGarguilo - Looking at the server configuration properties table, I like how the entries are more condensed and that there is a separator between each one. If we end up having a short description, and a longer explanation, then I can see how that might not work well in a table. Can the current format be adjusted to put a line separator between each metric, and to reduce the whitespace within the lines of the elements for each metric?

[DomGarguilo]

@DomGarguilo - Looking at the server configuration properties table, I like how the entries are more condensed and that there is a separator between each one. If we end up having a short description, and a longer explanation, then I can see how that might not work well in a table. Can the current format be adjusted to put a line separator between each metric, and to reduce the whitespace within the lines of the elements for each metric?

Yea I think those would be good improvements. I'll try to do that.

I think once this is merged and the follow on PR is created to fill out or improve all the descriptions, that will also give us a better idea of whether a table or "list" format would be better.

[DomGarguilo]

In e818f6d I added some more style changes so that the metrics page will render with alternating background colors per metric as well as reduced line break between fields:

I think this looks nice and did this as opposed to a line separator since there is one after each metrics section title (e.g. "General Server Metrics", "Compactor Metrics", etc.)

[dlmarion]

@DomGarguilo - this looks better, thanks for adding this change.