add low memory metric to abstract server

[EdColeman]

Adds metric to detect low memory conditions

• metric added to AbstractServer so all processes inherit the reporting of the value.
• includes minor ide checkstyle suggested fixes to tests.

[EdColeman]

Fixes issue #3243

[dlmarion]

This looks good, I'm glad that you added the new metric to one of the tests. I think you will also need to add the metric to the collection of unreported metrics in MetricsIT. Also, all of the classes that subclass AbstractServer (TabletServer, Manager, etc) call MetricsUtil.initializeProducers. I have not looked to see if there is an issue with calling it twice in the same process. I was originally thinking that in the subclasses of AbstractServer, we would just add this class as another producer to the list instead of calling it here too. Have you evaluated that already?

[ddanielr]

I'm not super familiar with Micrometer, but the way this is currently implemented, wouldn't
the metrics be created in the following structure?
accumulo.app.app1.server1.detected.low.memory = 1|0
accumulo.app.app1.server2.detected.low.memory = 1|0
accumulo.app.app2.server1.detected.low.memory = 1|0

I'm pretty sure these would all be considered unique metrics.

Does Micrometer support tags or metric labels?
If the application name and host name were added as tags on the metric, then the metric would look like
accumulo.app.detected.low.memory{"application Name": "app1", "hostname": "server1"} = 1|0

This drops the unique metric definition down to accumulo.app.detected.low.memory and allows sorting and filtering based on application name or hostname.

Alerting still works by firing on accumulo.app.detected.low.memory =1 but better granularity is allowed on the visualization and alerting ends.

[EdColeman]

There are tags - I'm trying to confirm that they are being set correctly as I look at Dave's other comments.

[dlmarion]

I agree, I think we want something like accumulo.server.detected.low.memory and the application name should be in a tag.

[EdColeman]

Leveraged tags in 674a9da

[dlmarion]

I think this was dropped accidentally. Compactor implements MetricsProducer too, so it's registerMetrics method is not getting called.

[EdColeman]

fixed in 49b93b3

[dlmarion]

I don't think this method is necessary, and it's initializing the metrics for AbstractServer differently than everything else. Since you have modified AbstractServer to implement MetricsProducer, then:

1. You should have any subclasses of AbstractServer call super.registerMetrics in their respective registerMetrics method. For example, Compactor.registerMetrics.
2. You should pass this in subclasses of AbstractServer in the call to MetricsUtil.initializeProducers.

[EdColeman]

@dlmarion What do you think is easier to understand? Classes that extended abstract server must call super.registerMetrics or call a method 'initServerMetrics' ? Either way requires something that may not be obvious.

Because of the tagging, the registration / initialization needs to occur in the run method - and after certain things have been initialized (host and port)

I'm not thrilled with either approach, but went with a specific initServerMetrics method - if relying on calling super.registerMetrics' and the needing to call the static MetricsUtil.initializeProducerswhich results in an indirect call toregisterMetrics`?

I was thinking about moving the static initializeMetrics out of MetricsUtil and put the functionality into AbstractServer - I think that functionality only applies to services extending AbstractServer.

The initializeProducers functionality needs to exist to register metrics producers for things that are created outside of the AbstarctServer context (thrift metrics, cache,....) - but those are created / exist within a service context that has already been expected to initialize the registry.

[dlmarion]

@dlmarion What do you think is easier to understand? Classes that extended abstract server must call super.registerMetrics or call a method 'initServerMetrics' ? Either way requires something that may not be obvious.

With Java, I think it's standard practice, or should be, for developers to evaluate whether or not the superclass method needs to be called when overriding a method.

Because of the tagging, the registration / initialization needs to occur in the run method - and after certain things have been initialized (host and port)

agreed

I'm not thrilled with either approach, but went with a specific initServerMetrics method - if relying on calling super.registerMetrics' and the needing to call the static MetricsUtil.initializeProducerswhich results in an indirect call toregisterMetrics`?

I was thinking about moving the static initializeMetrics out of MetricsUtil and put the functionality into AbstractServer - I think that functionality only applies to services extending AbstractServer.

I think that might be fine iff only AbstractServer subclasses call that method.

The initializeProducers functionality needs to exist to register metrics producers for things that are created outside of the AbstarctServer context (thrift metrics, cache,....) - but those are created / exist within a service context that has already been expected to initialize the registry.

agreed

[EdColeman]

Addressed in 49b93b3 - backed of removal of MetricsUtil for now to minimize changes. May look into additional changes in future PRs, but this code works as expected.

[EdColeman]

@dlmarion can you see if 49b93b3 contains the changes you requested? Or do you think there should be additional improvements?

[dlmarion]

I think with these changes overall, all subclasses of AbstractServer need to call MetricsUtil.initializeProducers, passing in this at a minimum. I think that CompactionCoordinator is missing this.