Provide a default metrics implementation using a LoggingProvider by EdColeman · Pull Request #4244 · apache/accumulo

EdColeman · 2024-02-08T00:18:37Z

Adds the MeterRegistryFactory to ..core.spi.metrics and deprecates the one in ..core.metrics
Modified the GENERAL_MICROMETER_FACTORY to be a list of class names. This would allow multiple provides to be used. For example, logging and statsd. Primarly this may help track down metrics issues.
set the default for mini cluster to log metrics. This should increase the visibility of metrics during development, but may be cluttering the logs.

This replaces #4218

- Adds the MeterRegistryFactory to .core.spi.metrics and deprecates the one in core.metrics - Modified the GENERAL_MICROMETER_FACTORY to be a list of class names. This would allow mutiple provides to be used. For example, logging and statsd. Primarly this may help track down metrics issues. - set the default for mini cluster to log metrics. This should increase the visibility of metrics during development, but may be cluttering the logs.

dlmarion · 2024-02-08T15:25:13Z

 logger.39.name = org.apache.accumulo.manager.Manager
 logger.39.level = trace

-property.metricsFilename = ./target/test-metrics


I'm wondering if we should undo this change now, so that the metrics go to a specific file for the ITs instead of polluting the server logs.

If we leave it, it needs reworked - that specific line you highlighted anyway - the log should be based on the service name (it's already a prop in the file) - hard coding it as test there helped when we tailed the logs so that it was fixed and well known, but maybe not the best example.

I'd prefer keeping it - or having it somewhere discover-able, because it is a pain to get it set up correctly when you are not configuring log4j regularly.

keith-turner · 2024-02-08T16:17:38Z

+  // defines the metrics update period, default is 60 seconds.
+  private final LoggingRegistryConfig lconf = c -> {
+    if (c.equals("logging.step")) {
+      return "60s";


Is there any way to configure this to change the frequency?

It is currently not a property - so no. 60s is actually the default, but specifically setting it here shows how to change it, if so inclined. I'm not sure how far we want to go with supporting this as a default - users should really be using a production solution, so bare-bones impl for this may be okay.

Being able to configure the class but not pass any config seems cumbersome to me. If a user configure their own factory and wants to set a server to pass metrics too are they expected to hard code the server address?

The config problem is not something to worry about this in PR as its a per-existing pattern. I am just wondering how someone would use actually use this feature and if it it requires recompiling java code for every metrics config change. If the end to end user experience could be improved we could consider that when introducing a new interface though.

FWIW, I used System properties in TestStatsDRegistryFactory. Not sure if that helps or not.

The TestStatsDRegistryFactory example splits config across two places. If a plugin follows this approach (its hard to think of any other approach a plugin could follow) then metrics config would end up being split across accumulo.properties file and accumulo-env.sh files. I will open a follow on issue about potentially improving this.

Opened #4262

keith-turner · 2024-02-08T16:20:13Z

+  GENERAL_MICROMETER_FACTORY("general.micrometer.factory",
+      "org.apache.accumulo.core.spi.metrics.SimpleLoggingMeterRegistryFactory",
+      PropertyType.CLASSNAMELIST,
+      "A comma separated list of one or more class names that implement MeterRegistryFactory.",


Suggested change

"A comma separated list of one or more class names that implement MeterRegistryFactory.",

"A comma separated list of one or more class names that implement MeterRegistryFactory. Prior to 2.1.3 the default value was blank. In 2.1.3 the default value was changed.",

It would be good to explain the change in expected interface class in the docs and what the support for the old interface class is.

Addressed in d8551b5

keith-turner · 2024-02-08T16:28:12Z

-      MeterRegistryFactory factory = clazz.getDeclaredConstructor().newInstance();
+      for (String factoryName : getTrimmedStrings(factoryClasses)) {
+        Class<? extends MeterRegistryFactory> clazz =
+            ClassLoaderUtil.loadClass(factoryName, MeterRegistryFactory.class);


If someone had the old Factory interface configured it seems like this would blow up.

Addressed in d8551b5

- handle deprecated factory classes - update property description.

keith-turner · 2024-02-14T15:17:14Z

+      PropertyType.CLASSNAMELIST,
+      "A comma separated list of one or more class names that implement MeterRegistryFactory. Prior to"
+          + " 2.1.3 this was a single value and the default was an empty string.  In 2.1.3 the default "
+          + " was changed and it now can accept multiple class names.",


Could also add something about the change in classes. Not sure if I got the packages correct.

In 2.1.3 a new type was introduced o.a.a.core.spi.metrics.MeterRegistryFacory and the existing type o.a.a.core.metrics.MeterRegistryFactory was deprecated. For now classes that extend either type are accepted as a value to this property.

EdColeman · 2024-02-14T17:13:49Z

I marked this as a blocker for 2.1.3. The SPI likely should go in some form into 2.1.3. Rather that merge this now, I working issue #4262 that could have additional improvements. If 2.1.3 release is ready, then what exists can be merged now and changes handled later.

…provider

EdColeman · 2024-04-13T00:23:27Z

Replaced with #4459

EdColeman requested review from ctubbsii and dlmarion February 8, 2024 00:18

remove metrics sample logging configuration

abc4de3

dlmarion reviewed Feb 8, 2024

View reviewed changes

remove invalid test stub

608490b

keith-turner reviewed Feb 8, 2024

View reviewed changes

Address PR comments

d8551b5

- handle deprecated factory classes - update property description.

dlmarion approved these changes Feb 12, 2024

View reviewed changes

keith-turner approved these changes Feb 14, 2024

View reviewed changes

keith-turner mentioned this pull request Feb 14, 2024

MeterRegistryFactory lacks mechanism for configuration. #4262

Closed

EdColeman added the blocker This issue blocks any release version labeled on it. label Feb 14, 2024

Merge remote-tracking branch 'upstream/2.1' into add_default_metrics_…

973b5b3

…provider

EdColeman mentioned this pull request Apr 13, 2024

Additional metrics changes #4459

Merged

EdColeman closed this Apr 13, 2024

EdColeman deleted the add_default_metrics_provider branch April 24, 2024 15:24

ctubbsii modified the milestones: 3.1.0, 2.1.3 Jul 12, 2024

	"A comma separated list of one or more class names that implement MeterRegistryFactory.",
	"A comma separated list of one or more class names that implement MeterRegistryFactory. Prior to 2.1.3 the default value was blank. In 2.1.3 the default value was changed.",

Conversation

EdColeman commented Feb 8, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdColeman commented Feb 14, 2024

Uh oh!

EdColeman commented Apr 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

keith-turner Feb 8, 2024 •

edited

Loading